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

Decrease padding right of AppNavigationCaption and ListItem to 4 px #2406

Closed
wants to merge 2 commits into from

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Dec 14, 2021

The right padding of AppNavigationCaption was to big, leading to a misalignment of the ActionMenus of AppNavigationItem and AppNavigationCaption. This PR adjusts the padding to the correct value, see
https://github.com/nextcloud/nextcloud-vue/blob/0173e9e9b14b12066415b0152eebfc67e32966ed/src/components/AppNavigationItem/AppNavigationItem.vue#L484

Before:
Screenshot 2021-12-14 at 21-53-09 Nextcloud Vue Style Guide

After:
Screenshot 2021-12-14 at 21-58-33 Nextcloud Vue Style Guide

@raimund-schluessler
Copy link
Contributor Author

/backport to stable4

Copy link

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Much better now!

Copy link
Contributor

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

This will make it out of alignment with the ListItem's actions though, which is what this component was matching. I would increase the appNavigationItem right padding instead.
Screenshot 2021-12-15 at 10 20 09

...Or, we could bring down the padding of the ListItem to 4 too, which I'm sure you'd love @raimund-schluessler :)

@raimund-schluessler
Copy link
Contributor Author

This will make it out of alignment with the ListItem's actions though

Hm, indeed, this won't work well:
Screenshot 2021-12-15 at 10-42-17 Nextcloud Vue Style Guide

which I'm sure you'd love @raimund-schluessler

@marcoambrosini I don't really have a preference here, to be honest. I just had the lengthy discussion in #1912 in mind where the right-padding: 4px; was done and thought that these 4 px are the desired value. But since I would just like them all to be aligned, I am fine with increasing the right-padding of AppNavigationItem to 8 px (although I am almost certain, this will mess up something else we don't think about at the moment 🙈 ).

@nextcloud/designers Should I adjust the PR to increase the padding of AppNavigationItem instead?

@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Dec 15, 2021

@nextcloud/designers Should I adjust the PR to increase the padding of AppNavigationItem instead?

For reference:

Everything with 8 px (increasing AppNavigationItem right padding):
8px

And with 4 px (decreasing ListItem and AppNavigationCaption right padding):
4px

Signed-off-by: Raimund Schlüßler <[email protected]>
@raimund-schluessler raimund-schluessler changed the title Adjust padding right of AppNavigationCaption Decrease padding right of AppNavigationCaption and ListItem to 4 px Dec 15, 2021
@raimund-schluessler
Copy link
Contributor Author

I adjusted this PR to change everything to 4 px:
4px

And I also created another PR #2407 adjusting everything to 8 px as comparison. So take your choice, the PR receiving more approvals will get merged 😉🙈

@raimund-schluessler
Copy link
Contributor Author

I will close this according to the discussion on c.nc.c. We will go for #2407 for now and completely rework the AppNavigationItem later.

@raimund-schluessler raimund-schluessler deleted the fix/noid/appnavigation-alignment branch December 15, 2021 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants