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: Make account menu follow the design (use nextcloud-vue components) #46880

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jul 30, 2024

Summary

Make the account menu use NcListItem to automatically follow the new design rules.

Screenshots

before after
Bildschirmfoto_20240730_144744 Bildschirmfoto_20240730_151009

Checklist

@susnux susnux added design Design, UI, UX, etc. 3. to review Waiting for reviews labels Jul 30, 2024
@susnux susnux added this to the Nextcloud 30 milestone Jul 30, 2024
@susnux susnux changed the title Fix/account menu fix: Make account menu follow the design (use nextcloud-vue components) Jul 30, 2024
@blizzz blizzz mentioned this pull request Jul 30, 2024
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.

Thanks @susnux! :) So it does look a bit strange, some detail points:

  • The icons seem to have grown, or is this an illusion?
  • Top padding above "admin" username is less than on the left, should be equidistant
  • The right margin (or padding) is very little, visible at "Appearance and accessibility"

@susnux
Copy link
Contributor Author

susnux commented Jul 31, 2024

Thanks @susnux! :) So it does look a bit strange, some detail points:

* [ ]  The icons seem to have grown, or is this an illusion?

I made them 20px but now reverted to 16px.

* [ ]  Top padding above "admin" username is less than on the left, should be equidistant

Was already the same as inline padding (4px grid baseline), you see this if the entry is active and get primary coloring. but adjusted a bit.

* [ ]  The right margin (or padding) is very little, visible at "Appearance and accessibility"

Increased both to 8px (2* grid baseline).

image

I personally think there is too much space between icon and text, but this comes from missing adjustments from shrinking clickable area / padding in the NcListItem component.

@marcoambrosini
Copy link
Member

@susnux why not action menu for this?

@blizzz blizzz mentioned this pull request Aug 1, 2024
@susnux
Copy link
Contributor Author

susnux commented Aug 1, 2024

why not action menu for this?

@marcoambrosini what do you mean? Naming? Or using NcActions?
If you mean NcActions, than we can not use that because they have more complex render logic and do not allow arbitrary content, but this menu does allow apps to register custom entries which is not possible with NcActions.

@Altahrim Altahrim mentioned this pull request Aug 5, 2024
@susnux
Copy link
Contributor Author

susnux commented Aug 5, 2024

@marcoambrosini @jancborchardt what do you think of the changes now? Can we go with it or should I adjust it?

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
fixup: Adjust to design comments

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux dismissed jancborchardt’s stale review August 6, 2024 07:40

Changes implemented

@susnux susnux merged commit f1cc819 into master Aug 6, 2024
111 checks passed
@susnux susnux deleted the fix/account-menu branch August 6, 2024 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: User menu in the header doesn't follow design changes
4 participants