Skip to content
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

fix(StatusMenu): StatusSuccessAction is not taken into account for Menu width #16357

Conversation

caybro
Copy link
Member

@caybro caybro commented Sep 18, 2024

What does the PR do

  • StatusSuccessAction, despite its name, is a visual item (MenuItem -> AbstractButton) which is not part of the contentModel but just added to the menu container
  • therefore we don't use a ListView but a ScrollView/Repeater instead and set the width/maxWidth manually after the menu items have been added to the layout

Fixes #14037

Affected areas

Context Menus

Architecture compliance

  • I am familiar with the application architecture and agreed good practices.
    My PR is consistent with this document: Architecture guidelines

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it

image

@@ -67,16 +67,6 @@ Menu {
property var openHandler
property var closeHandler

function checkIfEmpty() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code

@status-im-auto
Copy link
Member

status-im-auto commented Sep 18, 2024

Jenkins Builds

Click to see older builds (25)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ f070fb7 #1 2024-09-18 08:37:08 ~6 min tests/nim 📄log
✔️ f070fb7 #1 2024-09-18 08:38:28 ~7 min macos/aarch64 🍎dmg
✔️ f070fb7 #1 2024-09-18 08:42:42 ~12 min tests/ui 📄log
✔️ f070fb7 #1 2024-09-18 08:46:07 ~15 min linux-nix/x86_64 📦tgz
✔️ f070fb7 #1 2024-09-18 08:46:15 ~15 min macos/x86_64 🍎dmg
✔️ f070fb7 #1 2024-09-18 08:47:27 ~17 min linux/x86_64 📦tgz
✔️ 091d8a3 #2 2024-09-19 07:03:04 ~4 min macos/aarch64 🍎dmg
✔️ 091d8a3 #2 2024-09-19 07:05:12 ~6 min tests/nim 📄log
091d8a3 #2 2024-09-19 07:09:59 ~11 min tests/ui 📄log
✔️ 091d8a3 #2 2024-09-19 07:10:09 ~11 min macos/x86_64 🍎dmg
✔️ 091d8a3 #2 2024-09-19 07:12:53 ~14 min linux-nix/x86_64 📦tgz
✔️ 091d8a3 #2 2024-09-19 07:13:16 ~14 min linux/x86_64 📦tgz
✔️ 091d8a3 #3 2024-09-19 07:38:45 ~11 min tests/ui 📄log
✔️ 5748284 #3 2024-09-23 08:01:04 ~4 min macos/aarch64 🍎dmg
✔️ 5748284 #3 2024-09-23 08:03:20 ~6 min tests/nim 📄log
✔️ 5748284 #4 2024-09-23 08:08:23 ~11 min tests/ui 📄log
✔️ 5748284 #3 2024-09-23 08:08:23 ~11 min macos/x86_64 🍎dmg
✔️ 5748284 #3 2024-09-23 08:09:37 ~13 min linux-nix/x86_64 📦tgz
✔️ 5748284 #3 2024-09-23 08:11:33 ~14 min linux/x86_64 📦tgz
✔️ b049fbc #4 2024-09-23 10:46:29 ~4 min macos/aarch64 🍎dmg
✔️ b049fbc #4 2024-09-23 10:48:45 ~6 min tests/nim 📄log
✔️ b049fbc #5 2024-09-23 10:53:47 ~11 min tests/ui 📄log
✔️ b049fbc #4 2024-09-23 10:54:01 ~12 min macos/x86_64 🍎dmg
✔️ b049fbc #4 2024-09-23 10:55:58 ~14 min linux-nix/x86_64 📦tgz
✔️ b049fbc #4 2024-09-23 10:56:55 ~14 min linux/x86_64 📦tgz
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 866e4a0 #5 2024-09-23 11:57:53 ~4 min macos/aarch64 🍎dmg
✔️ 866e4a0 #5 2024-09-23 12:00:12 ~6 min tests/nim 📄log
✔️ 866e4a0 #5 2024-09-23 12:03:54 ~10 min macos/x86_64 🍎dmg
✔️ 866e4a0 #6 2024-09-23 12:05:42 ~12 min tests/ui 📄log
✔️ 866e4a0 #5 2024-09-23 12:06:57 ~13 min linux-nix/x86_64 📦tgz
✔️ 866e4a0 #5 2024-09-23 12:08:43 ~15 min linux/x86_64 📦tgz
✔️ b265427 #6 2024-09-23 13:52:48 ~4 min macos/aarch64 🍎dmg
✔️ b265427 #6 2024-09-23 13:54:43 ~6 min tests/nim 📄log
✔️ b265427 #6 2024-09-23 13:59:20 ~10 min macos/x86_64 🍎dmg
✔️ b265427 #7 2024-09-23 14:00:02 ~11 min tests/ui 📄log
✔️ b265427 #6 2024-09-23 14:01:22 ~13 min linux-nix/x86_64 📦tgz
✔️ b265427 #6 2024-09-23 14:03:24 ~15 min linux/x86_64 📦tgz

@status-im-auto
Copy link
Member

ui/StatusQ/src/StatusQ/Popups/StatusMenu.qml Outdated Show resolved Hide resolved
Copy link
Member

@micieslak micieslak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some investigation I've found some alternative solution, probably worth to consider. Please check in the comment.

ui/StatusQ/src/StatusQ/Popups/StatusMenu.qml Outdated Show resolved Hide resolved
ui/StatusQ/src/StatusQ/Popups/StatusMenu.qml Outdated Show resolved Hide resolved
@caybro caybro force-pushed the 14037-statusmenu-statussuccessaction-is-not-taking-into-account-text-content-width branch from f070fb7 to 091d8a3 Compare September 19, 2024 06:58
@status-im-auto
Copy link
Member

Copy link
Member

@micieslak micieslak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@anastasiyaig
Copy link
Contributor

i am looking at autotests now

@anastasiyaig
Copy link
Contributor

anastasiyaig commented Sep 23, 2024

@caybro i fixed the locator and tests are working fine now. However, i found one thing that propbably is broken in this PR:

screenshot_7LMkeHgrc2

you need to create account, logout and open app again, when being on the login screen - open the accounts drop down
could you pls check?

WRN 2024-09-23 11:04:37.637+03:00 qt warning                                 topics="qt" tid=14085456 text="QQuickItem::stackBefore: Cannot stack AccountMenuItemPanel_QMLTYPE_3021(0x6000013e6680, parent=0x60000110fc00, geometry=0,0 342x64) before QQuickRepeater(0x6000013b3a70, name = \"LoginView_AccountsRepeater\"), which must be a sibling" file=:0 category=default

@status-im-auto
Copy link
Member

…nu width

- StatusSuccessAction, despite its name, is a visual item (`MenuItem` ->
`AbstractButton`) which is not part of the `contentModel` but just added
to the menu container
- therefore we don't use a ListView but a ScrollView/Repeater instead
and set the width/maxWidth manually after the menu items have been added
to the layout

Fixes #14037
@caybro caybro force-pushed the 14037-statusmenu-statussuccessaction-is-not-taking-into-account-text-content-width branch from 5748284 to b049fbc Compare September 23, 2024 10:41
@status-im-auto
Copy link
Member

@status-im-auto
Copy link
Member

@anastasiyaig
Copy link
Contributor

alright, seems one more fix is required, checking

@anastasiyaig anastasiyaig force-pushed the 14037-statusmenu-statussuccessaction-is-not-taking-into-account-text-content-width branch from 866e4a0 to b265427 Compare September 23, 2024 13:48
@status-im-auto
Copy link
Member

@anastasiyaig
Copy link
Contributor

its good to go now, all fixed

@caybro caybro merged commit 9abc014 into master Sep 23, 2024
9 checks passed
@caybro caybro deleted the 14037-statusmenu-statussuccessaction-is-not-taking-into-account-text-content-width branch September 23, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[StatusMenu] StatusSuccessAction is not taking into account text content Width
5 participants