-
Notifications
You must be signed in to change notification settings - Fork 85
fix(StatusPopupMenu): Refactoring #8341
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
Conversation
Jenkins Builds
|
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.
Code looks good, just some minor nits
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.
Nice delegate simplification 👍 left some comments
closePolicy: Popup.CloseOnPressOutside | Popup.CloseOnEscape | ||
topPadding: 8 | ||
bottomPadding: 8 | ||
bottomMargin: 16 | ||
|
||
property int menuItemCount: 0 | ||
property var subMenuItemIcons: [] | ||
property StatusAssetSettings assetSettings: StatusAssetSettings { |
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.
IMO it doesn't belong here. It should be defined by the delegate.
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.
color: "transparent" | ||
} | ||
|
||
property StatusFontSettings fontSettings: StatusFontSettings {} |
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.
Same as above.
dc3ea0e
to
426ecf9
Compare
Part of #7232
As continuation of status-im/StatusQ#894.
This is a PR to a temporary branch
fix/status-menu
After this PR is merged, I will update and test the main application. And create a PR to
master
.StatusQ checklist
Screenshot of functionality (including design for comparison)
2022-11-21.11.44.28.mov