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

feat: Add NcAppNavigationList #5302

Merged
merged 1 commit into from Feb 27, 2024
Merged

feat: Add NcAppNavigationList #5302

merged 1 commit into from Feb 27, 2024

Conversation

Pytal
Copy link
Contributor

@Pytal Pytal commented Feb 24, 2024

☑️ Resolves

  • There are cases where having multiple uls inside NcAppNavigation makes sense i.e. using NcAppNavigationCaption as a label for the next ul
  • We add a new NcAppNavigationList component to be used in the default slot of NcAppNavigation
  • Allow using NcAppNavigationCaption as a heading with a prop

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@Pytal Pytal added enhancement New feature or request 3. to review Waiting for reviews feature: app-navigation Related to the app-navigation component accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Feb 24, 2024
@Pytal Pytal added this to the 8.8.0 milestone Feb 24, 2024
@Pytal Pytal self-assigned this Feb 24, 2024
@@ -66,7 +66,7 @@ emit('toggle-navigation', {
@keydown.esc="handleEsc">
<slot />
<!-- List for Navigation li-items -->
<ul class="app-navigation__list">
<ul v-if="hasListSlotItems" class="app-navigation__list">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is ul.app-navigation__list different from NcAppNavigationList?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also stumbled over this one, could we not just use NcAppNavigationList here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is ul.app-navigation__list different from NcAppNavigationList?

Yes slightly. The difference is ul.app_navigation__list always fills the height that is not already taken up by its siblings inside nav.app-navigation__content but when we have separate lists inside the nav we want don't want that e.g.

image

so we adjust the styles in NcAppNavigationList to keep the styling we want:

image

The .app_navigation__list styles also target .app_navigation__content > ul which we should keep to not break anything, so we also increase the specificity to have the styles apply correctly

src/components/NcAppNavigation/NcAppNavigation.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

great refactoring work!

Signed-off-by: Christopher Ng <chrng8@gmail.com>
@Pytal Pytal merged commit c1c5bb6 into master Feb 27, 2024
18 checks passed
@Pytal Pytal deleted the feat/app-nav-list branch February 27, 2024 01:26
@Pytal
Copy link
Contributor Author

Pytal commented Feb 27, 2024

/backport to next

@backportbot backportbot bot mentioned this pull request Feb 27, 2024
2 tasks
@Antreesy Antreesy mentioned this pull request Feb 29, 2024
@ShGKme ShGKme mentioned this pull request Feb 29, 2024
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 accessibility Making sure we design for the widest range of people possible, including those who have disabilities enhancement New feature or request feature: app-navigation Related to the app-navigation component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants