fix(StatusSearchLocationMenu): Refactor using new StatusPopupMenu
#8436
Conversation
Jenkins BuildsClick to see older builds (15)
|
igor-sirotin
force-pushed
the
fix/7232-serach-location-menu
branch
from
1c2b33e to
364141b
Compare
igor-sirotin
changed the title
StatusSearchLocationMenu. Rename components. StatusSearchLocationMenu
igor-sirotin
changed the title
StatusSearchLocationMenuStatusPopupMenu
igor-sirotin
force-pushed
the
fix/7232-serach-location-menu
branch
from
09e90b1 to
d9d0764
Compare
igor-sirotin
force-pushed
the
fix/7232-serach-location-menu
branch
from
d9d0764 to
f931d96
Compare
Thanks! |
There was a problem hiding this comment.
LGTM. I' d just like one of the members of the UI cohort give a looks since there are a lot of StatusQ changes. cc @caybro
| menu.removeMenu(object) | ||
| else if (object instanceof Action) | ||
| menu.removeAction(object) | ||
| else // if (object instanceof MenuItem) |
There was a problem hiding this comment.
Is the comment still needed?
There was a problem hiding this comment.
Nope, it's not. Thanks for pointing.
I removed it locally, will push is as part of next PR, not to block this one 🙂
There was a problem hiding this comment.
@jrainville, added here: #8473
| name: root.text | ||
| letterSize: 11 | ||
| readonly property StatusIdenticonRingSettings defaultRingSettings: StatusIdenticonRingSettings { | ||
| ringPxSize: Math.max(1.5, d.assetSettings.width / 24.0) |
There was a problem hiding this comment.
Not sure if worth it, but we seem to be doing the same thing here: https://github.com/status-im/status-desktop/pull/8436/files#diff-a295c8811d526e0c83e97c3c9431b689b5cce6e9ff2b0c73bcc39b003a7a18edR31 maybe it makes sense to have a little utils function for that so we ensure this doesn't get out of sync when changes are made?
There was a problem hiding this comment.
Good point, thanks! Found it in 5 places 🤦
Added it as part of next PR #8473, not to block this one
| import StatusQ.Core.Utils 0.1 as StatusQUtils | ||
|
|
||
| StatusPopupMenu { | ||
| id: root | ||
| dim: false |
There was a problem hiding this comment.
I assume we're getting a dim: false experience by other means now?
There was a problem hiding this comment.
Yeap, we have it in the StatusPopupMenu. And always had actually 🙂
Part of #7232
As continuation of #8341
What does the PR do
StatusSearchLocationMenuringSettingsStatusMenuItemDelegateto useStatusSmartIdenticonStatusSpellcheckingMenuItemsas unusedStatusMenuInstantiatorfor creating menu elements from a modelThis is a PR to a temporary branch
fix/status-menuI'm publishing changes one by one to make the code review process easier
StatusQ checklist
Screenshot of functionality (including design for comparison)