-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: add NcAppNavigationSettingsButton #5866
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Grigorii K. Shartsev <[email protected]>
…rgin from global vars Signed-off-by: Grigorii K. Shartsev <[email protected]>
…gation Signed-off-by: Grigorii K. Shartsev <[email protected]>
|
||
&__text { | ||
// Make the settings button less presentative, like app navigation items | ||
font-weight: normal; |
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.
This feels every different to all other elements and is different to all current implementations, I would like to ask @nextcloud-libraries/designers first
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.
This feels every different to all other elements and is different to all current implementations
I actually agree, and that's exactly what we were discussing yesterday.
But from yesterday's design review call:
- It should not be bold or secondary, otherwise it is too presentive, more than other items in the menu.
- It should be the same in all navigation menus, no matter if
NcAppNavigationItem
orNcListItem
is used - It should be aligned with navigation items (
NcAppNavigationItem
)
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.
To confirm – what @ShGKme listed is what I said the requirements are. The design in this PR fulfills all those requirements and looks most consistent of the possible solutions.
<docs> | ||
⚠️ This component is deprecated and will be removed in v9. | ||
|
||
Use `<NcAppNavigationSettingsButton>` with `<NcAppSettingsButton>` instead. |
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.
NcAppSettingsButton
? You mean NcAppSettingsDialog
?
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.
Yes
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.
Design-wise this looks very nice and consistent! Just some details:
- In Files it looked great previously, with same spacing between the 3 elements on the bottom (quota, deleted files and settings), now Settings moved a bit down, leaving a gap instead of being equidistant.
- The placement does not look exactly the same in all of the apps, if you compare all the "After" screenshots. In Files, Photos and Talk it is further down than in Calendar and Contacts (is this cause they don’t use the new 34px yet?)
☑️ Resolves
NcAppNavigationItem
, has padding.More advanced component is coming soon ✨
🖼️ Screenshots
🚧 Tasks
NcAppNavigationSettings
as deprecatedNcAPpNavigationSettingsButton
🏁 Checklist
next
requested with a Vue 3 upgrade