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

fix(NcAppNavigation): add border-right #5953

Merged
merged 1 commit into from
Aug 20, 2024
Merged

fix(NcAppNavigation): add border-right #5953

merged 1 commit into from
Aug 20, 2024

Conversation

marcoambrosini
Copy link
Contributor

No description provided.

@marcoambrosini marcoambrosini added bug Something isn't working 3. to review Waiting for reviews design Design, UX, interface and interaction design labels Aug 13, 2024
@marcoambrosini marcoambrosini self-assigned this Aug 13, 2024
@marcoambrosini marcoambrosini changed the title Fix: add navigation border Fix(NcAppNavigation): add border-right Aug 13, 2024
@marcoambrosini marcoambrosini force-pushed the feat/add-navigation-border branch from a144340 to 3a5d01b Compare August 13, 2024 12:21
@marcoambrosini marcoambrosini added this to the 9.0.0 milestone Aug 13, 2024
@marcoambrosini marcoambrosini requested a review from susnux August 14, 2024 08:06
@susnux
Copy link
Contributor

susnux commented Aug 14, 2024

The commit sign-off needs to be fixed, otherwise good!

@ShGKme
Copy link
Contributor

ShGKme commented Aug 15, 2024

Before After
image image
image image
image image
image image

ShGKme
ShGKme approved these changes Aug 15, 2024
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Better, but for me it makes it even less easy to find the toggle button because not it is visually out of the navigation.

@ShGKme ShGKme modified the milestones: 9.0.0, 8.16.1 Aug 15, 2024
@ShGKme
Copy link
Contributor

ShGKme commented Aug 15, 2024

/backport to next

@susnux
Copy link
Contributor

susnux commented Aug 15, 2024

for me it makes it even less easy to find the toggle button because not it is visually out of the navigation.

I think as we overlay the content a shadow might be better suited:

Bildschirmaufnahme_20240815_121458.webm

But still this PR makes things lot better :)

@ShGKme ShGKme changed the title Fix(NcAppNavigation): add border-right fix(NcAppNavigation): add border-right Aug 20, 2024
@marcoambrosini @susnux @ShGKme
fix(NcAppNavigation): add border right
3bf8bae 
Signed-off-by: Marco Ambrosini <marcoambrosini@proton.me>
Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de>
@ShGKme ShGKme force-pushed the feat/add-navigation-border branch from d562637 to 3bf8bae Compare August 20, 2024 08:39
@ShGKme
Copy link
Contributor

ShGKme commented Aug 20, 2024

Fixed DCO and squashed

@ShGKme
Copy link
Contributor

ShGKme commented Aug 20, 2024

Merge?

@marcoambrosini marcoambrosini merged commit 8ecdf87 into master Aug 20, 2024
19 checks passed
@marcoambrosini marcoambrosini deleted the feat/add-navigation-border branch August 20, 2024 08:43
@backportbot backportbot bot mentioned this pull request Aug 20, 2024
Merged
@susnux susnux mentioned this pull request Aug 20, 2024
Merged
@susnux susnux modified the milestones: 8.16.1, 8.17.0 Aug 20, 2024
@ShGKme ShGKme mentioned this pull request Aug 22, 2024
Merged
4 tasks
@Stanislav1975 Stanislav1975 mentioned this pull request Sep 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShGKme ShGKme ShGKme approved these changes

@susnux susnux susnux approved these changes

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

Assignees

@marcoambrosini marcoambrosini

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

Successfully merging this pull request may close these issues.
3 participants
@marcoambrosini @susnux @ShGKme