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

Properly align AppNavigationToggle with first navigation item #3278

Merged
merged 1 commit into from Oct 4, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Sep 20, 2022

Currently the toggle is set at 4px while the app navigation items are placed at 8px (using that variable), this looks slightly displaced.

So this PR moves the padding calculation into a variable and uses the variable for the toggle (besides the navigation items).

Before / Currently:
toggle slightly misplaced

After / This PR:
Toggle centered

@susnux susnux added 3. to review Waiting for reviews feature: app-navigation Related to the app-navigation component design Design, UX, interface and interaction design labels Sep 20, 2022
@ChristophWurst
Copy link
Contributor

I'm not able to see a difference between the before and after screenshot

@susnux
Copy link
Contributor Author

susnux commented Sep 21, 2022

I'm not able to see a difference between the before and after screenshot

Have a close look on the navigation toggle, on the before screenshot it is slightly of-center (4px displaced to the top).
As the navigation items use a padding of 8px => spaced 8px from the top, but the navigation toggle uses a margin of 4px.

@ChristophWurst
Copy link
Contributor

@GretaD please test how this aligns with the Groupware apps

@GretaD
Copy link
Contributor

GretaD commented Sep 21, 2022

on main
Screenshot from 2022-09-21 19-04-52

on this pr is slightly down(red background is added so i can tell im connected to nc/vue, ignore that)
if we want to keep this change, we can change the custom padding for search box to fit in. Same for mail with the button.
Screenshot from 2022-09-21 19-02-35

@susnux
Copy link
Contributor Author

susnux commented Sep 23, 2022

My reason why I think fixing the alignment here would be better then manually align it in every project is that many apps use the default styling, so e.g. even the user settings (/settings/users) page is affected:

offset of toggle to first nav. item

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 24, 2022

This issue has been here for a while, talk shift it manually, photos too, having a proper standard would indeed be nice yeah :)
If we cannot agree on a placement that fits all apps, please keep using a css variable (or a prop) so it's easier to adjust for all apps

@susnux
Copy link
Contributor Author

susnux commented May 31, 2023

Rebased this and used a css variable for easier adjustment as wished.

I think with current master being version 8 for NC28 we give apps enough time to adjust.

@susnux susnux requested review from raimund-schluessler and ShGKme and removed request for marcoambrosini May 31, 2023 17:05
@susnux susnux added this to the 8.0.0 milestone May 31, 2023
@susnux
Copy link
Contributor Author

susnux commented Sep 26, 2023

Ping?

…avigation item

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux susnux merged commit 37749e5 into master Oct 4, 2023
15 checks passed
@susnux susnux deleted the fix/align-nav-toggle branch October 4, 2023 15:36
@Pytal Pytal added the bug Something isn't working label Oct 6, 2023
@Pytal Pytal mentioned this pull request Oct 6, 2023
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 bug Something isn't working design Design, UX, interface and interaction design feature: app-navigation Related to the app-navigation component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants