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.

Sign up for GitHub

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

Jump to bottom

fix(core): app menu label position and animation #46916

Merged
merged 2 commits into from
Jul 31, 2024
Merged

fix(core): app menu label position and animation #46916

merged 2 commits into from
Jul 31, 2024

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jul 31, 2024 •
edited
Loading

Fix #46542

Before After
Peek 31-07-2024 15-14 Peek 31-07-2024 15-12
With notification ➡️ Peek 31-07-2024 17-19

@skjnldsv skjnldsv added bug design Design, UI, UX, etc. 3. to review Waiting for reviews 30-feedback labels Jul 31, 2024
@skjnldsv skjnldsv added this to the Nextcloud 30 milestone Jul 31, 2024
@skjnldsv skjnldsv self-assigned this Jul 31, 2024
@skjnldsv skjnldsv requested review from Fenn-CS and sorbaugh and removed request for a team July 31, 2024 13:19
ChristophWurst
ChristophWurst approved these changes Jul 31, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Not my area of expertise but looks good on the videos 👍

@susnux
Copy link
Contributor

susnux commented Jul 31, 2024 •
edited
Loading

I think they are too narrow now, try to add a unread notification and it will overflow the text, you see it on the activity app icon: Label and icon do not have any spacing, but the unread notification will cover -5px.

@skjnldsv
Copy link
Member Author

skjnldsv commented Jul 31, 2024 •
edited
Loading

I think they are too narrow now, try to add a unread notification and it will overflow the text, you see it on the activity app icon: Label and icon do not have any spacing, but the unread notification will cover -5px.

I used the exact screenshots from a 28 29 instance
image

@susnux
Copy link
Contributor

susnux commented Jul 31, 2024

I used the exact screenshots from a 28 29 instance

And that is what I tried to fix with the initial PR 😅

susnux
susnux approved these changes Jul 31, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Approve as it restores NC29 design, but please see my comment about the missing padding for notification :)

@skjnldsv
Copy link
Member Author

Feel free to take over @susnux 🙈

@skjnldsv
Copy link
Member Author

/compile rebase-amend /

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 31, 2024
@skjnldsv @nextcloud-command
fix(core): app menu label position and animation
937bb4a 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@skjnldsv @nextcloud-command
fix(core): do not show unread notification on app menu hover
b92e0c7 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@skjnldsv skjnldsv enabled auto-merge July 31, 2024 15:31
@skjnldsv skjnldsv merged commit 8b238a2 into master Jul 31, 2024
111 checks passed
@skjnldsv skjnldsv deleted the fix/appmenu-label-pos branch July 31, 2024 16:08
jancborchardt
jancborchardt reviewed Jul 31, 2024
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Notification bubble should be in the top right, as it is e.g. on iOS, Android and for our own notification icon as well. Then there is also no text overlap issue. cc @susnux in case it got lost in the other issue. :)

@skjnldsv
Copy link
Member Author

@jancborchardt, I'll address it in a PR right now so it doesn't get lost

@skjnldsv skjnldsv mentioned this pull request Jul 31, 2024
Closed
@blizzz blizzz mentioned this pull request Aug 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jancborchardt jancborchardt jancborchardt left review comments

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@susnux susnux susnux approved these changes

@Fenn-CS Fenn-CS Awaiting requested review from Fenn-CS Fenn-CS was automatically assigned from nextcloud/server-frontend

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh was automatically assigned from nextcloud/server-frontend

Assignees

@skjnldsv skjnldsv

Labels
4. to release Ready to be released and/or waiting for tests to finish 30-feedback bug design Design, UI, UX, etc. regression
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

[Bug]: Increased space between apps and name when hovering the apps menu
4 participants
@skjnldsv @susnux @jancborchardt @ChristophWurst