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

Make app navigation settings pixel perfect-ish #3256

Merged
merged 1 commit into from
Sep 15, 2022
Merged

Make app navigation settings pixel perfect-ish #3256

merged 1 commit into from
Sep 15, 2022

Conversation

ChristophWurst
Copy link
Contributor

@ChristophWurst ChristophWurst commented Sep 14, 2022 •
edited
Loading

Contributes to #3184.

Follow-up to #3158.

  • Make settings button same with as other nav elements
  • Make sure scrolled menus don't look cut at the bottom
Description Before After
General Bildschirmfoto vom 2022-09-14 14-45-35 Bildschirmfoto vom 2022-09-14 14-43-38
Matching width Bildschirmfoto vom 2022-09-14 14-45-51 Bildschirmfoto vom 2022-09-14 14-43-35
Scrolling Bildschirmfoto vom 2022-09-14 14-45-56 Bildschirmfoto vom 2022-09-14 14-43-45

@ChristophWurst ChristophWurst added bug Something isn't working 3. to review Waiting for reviews design Design, UX, interface and interaction design labels Sep 14, 2022
@ChristophWurst ChristophWurst added this to the 7.0.0 milestone Sep 14, 2022
@ChristophWurst ChristophWurst self-assigned this Sep 14, 2022
@ChristophWurst ChristophWurst mentioned this pull request Sep 14, 2022
Merged
4 tasks
vinicius73
vinicius73 approved these changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@vinicius73 vinicius73 left a comment

Choose a reason for hiding this comment

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

Nice trick.

It is possible to use a variable to represent this 3px ?
Just to enable future adjusts directly from themes.

@ChristophWurst
Copy link
Contributor Author

It is possible to use a variable to represent this 3px ?
Just to enable future adjusts directly from themes.

Possibly but I don't know what is best practice for components like ours.

vinicius73
vinicius73 reviewed Sep 14, 2022
View reviewed changes
Copy link
Contributor

@vinicius73 vinicius73 left a comment

Choose a reason for hiding this comment

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

Check if it makes sense.

src/components/NcAppNavigationSettings/NcAppNavigationSettings.vue Outdated Show resolved Hide resolved
src/components/NcAppNavigationSettings/NcAppNavigationSettings.vue Outdated Show resolved Hide resolved
src/components/NcAppNavigationSettings/NcAppNavigationSettings.vue Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Contributor Author

I was able to solve this with a scss variable. Thanks for the input @vinicius73 :)

@ChristophWurst ChristophWurst force-pushed the fix/pixel-perfect-app-navigation-settings branch from d12ebe1 to d52e85d Compare September 15, 2022 07:38
@PVince81 PVince81 merged commit b83c16d into master Sep 15, 2022
@PVince81 PVince81 deleted the fix/pixel-perfect-app-navigation-settings branch September 15, 2022 11:52
@Pytal Pytal mentioned this pull request Sep 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinicius73 vinicius73 vinicius73 approved these changes

@max-nextcloud max-nextcloud max-nextcloud approved these changes

@raimund-schluessler raimund-schluessler Awaiting requested review from raimund-schluessler

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@mejo- mejo- Awaiting requested review from mejo-

@artonge artonge Awaiting requested review from artonge

@GretaD GretaD Awaiting requested review from GretaD

Assignees

@ChristophWurst ChristophWurst

Labels
3. to review Waiting for reviews bug Something isn't working design Design, UX, interface and interaction design
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ChristophWurst @vinicius73 @max-nextcloud @PVince81