Skip to content

fix(NcAppNavigationItem): multi level padding #6861

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented Apr 28, 2025

☑️ Resolves

🖼️ Screenshots

🏚️ Before 🏡 After
Screenshot from 2025-04-28 12-18-14 Screenshot from 2025-04-28 13-46-58

🚧 Tasks

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 2️⃣ Backport to stable8 for maintained Vue 2 version or not applicable

@GretaD GretaD self-assigned this Apr 28, 2025
@GretaD GretaD added bug Something isn't working 2. developing Work in progress regression Regression of a previous working feature labels Apr 28, 2025
@ShGKme ShGKme added feature: app-navigation Related to the app-navigation component and removed regression Regression of a previous working feature labels Apr 28, 2025
@ShGKme ShGKme changed the title fix: multilevel mailbox indentation fix(NcAppNavigationItem): multi level padding Apr 28, 2025
@GretaD
Copy link
Contributor Author

GretaD commented May 14, 2025

/backport! to stable8

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.

The level should be limited (or add scrolling), otherwise there is no place for deep nested items.

@@ -132,11 +132,12 @@
flex-direction: column;
width: 100%;
gap: var(--default-grid-baseline, 4px);
padding-left: 10px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the next line, it should be -inline-start to support RTL


.app-navigation-entry {
display: inline-flex;
flex-wrap: wrap;
padding-inline-start: $icon-size;
padding-inline-start: 10px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i changed this to 10px, instead of 16px (icon-size), because the indentation is unnecessarily large.

@GretaD GretaD added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 14, 2025
@GretaD GretaD requested review from ShGKme and susnux May 14, 2025 12:47
@ShGKme
Copy link
Contributor

ShGKme commented May 14, 2025

The level should be limited (or add scrolling), otherwise there is no place for deep nested items.

image

@susnux
Copy link
Contributor

susnux commented May 14, 2025

The level should be limited (or add scrolling), otherwise there is no place for deep nested items.

I do not remember where but I think we had discussed this for files someday and designers mentioned a cap of 3rd level.

@ShGKme
Copy link
Contributor

ShGKme commented May 14, 2025

I do not remember where but I think we had discussed this for files someday and designers mentioned a cap of 3rd level.

Or 7th?

https://github.com/nextcloud/server/blob/1b72ddd8c8aa187a6723a93575643a96f9191387/apps/files/src/components/FilesNavigationItem.vue#L51

@ShGKme
Copy link
Contributor

ShGKme commented May 14, 2025

Proposals for nesting limit:

  1. CSS solution - use CSS variable for the padding and reset it on a specific level
.app-navigation-entry__children {
  & & & & & & & {
    --app-navigation-item-child-offset: 0;
  }
}
  1. Vue solution - provide depth and set class depending on the depth:
const depth = inject('NcAppNavigationItem:depth', 0) + 1
provide('NcAppNavigationItem:depth', depth)

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 feature: app-navigation Related to the app-navigation component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sub mailboxes leveling indentation is off
3 participants