-
Notifications
You must be signed in to change notification settings - Fork 90
Part of Emoji Reaction final design #19140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
25fd704
05aa7b9
e034f0c
18e6aab
fb48c88
ddb87db
dd1b1c1
d92a777
82125e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,32 +2,33 @@ import QtQuick | |
|
|
||
| import StatusQ.Core | ||
| import StatusQ.Core.Theme | ||
| import StatusQ.Components | ||
|
|
||
| import utils | ||
| import shared | ||
| import shared.panels | ||
|
|
||
| Rectangle { | ||
| property alias source: reactionImage.source | ||
| property string emoji | ||
| required property string emojiId | ||
| property bool reactedByUser: false | ||
| property bool isHovered: false | ||
| signal closeModal() | ||
| signal toggleReaction() | ||
|
|
||
| id: root | ||
| width: reactionImage.width + Theme.halfPadding | ||
| width: statusEmoji.width + Theme.halfPadding | ||
| height: width | ||
| color: reactedByUser ? Theme.palette.secondaryBackground : | ||
| (isHovered ? Theme.palette.backgroundHover : Theme.palette.transparent) | ||
| border.width: reactedByUser ? 1 : 0 | ||
| border.color: Theme.palette.primaryColor1 | ||
| radius: Theme.radius | ||
|
|
||
| SVGImage { | ||
| id: reactionImage | ||
| width: 32 | ||
| fillMode: Image.PreserveAspectFit | ||
| StatusEmoji { | ||
| id: statusEmoji | ||
| anchors.centerIn: parent | ||
| width: 20 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want a fixed size or something dynamic for the times coming? ;)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I followed the design, but you're right, the design was for Desktop only. What do you suggest to make it more dynamic?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed value is far from ideal but the question is what should be the base for that size. Maybe it could be derived from some font size (from Theme of), done using |
||
| height: 20 | ||
| emojiId: root.emojiId | ||
| } | ||
|
|
||
| StatusMouseArea { | ||
|
|
@@ -36,8 +37,6 @@ Rectangle { | |
| hoverEnabled: !reactedByUser | ||
| onEntered: root.isHovered = true | ||
| onExited: root.isHovered = false | ||
| onClicked: { | ||
| root.closeModal(); | ||
| } | ||
| onClicked: root.toggleReaction() | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be cleaner to actually have a small
Settings {}inside the popup and encapsulate theskinColorandrecentEmojisentirely inside the popup. I'd consider that way cleaner than it is now; some of those settigns are handled inside the popup (in the opened/closed handlers), and some outside hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the opposite side - externalizing gives better flexibility, reusability and it's easier/cleaner to test. API is then big wider, but it's what actually the component needs.