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

fix(NcAppNavigation): Wrap app navigation default slot with scrollable container #5347

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

Pytal
Copy link
Contributor

@Pytal Pytal commented Mar 5, 2024

☑️ Resolves

  • To fix the unscrollable content in NcAppNavigationList we wrap the entire default slot with a container
  • The div.app-navigation__body now fills the height with height: 100%
    • Remove the redundant &:nth-last-of-type(2) fill styles
  • NcAppNavigationList now has height: fit-content in all contexts and is itself not scrollable now that scrolling is delegated to the container similar to the ul.app-navigation__list list slot behaviour

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🏁 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 bug Something isn't working 3. to review Waiting for reviews feature: app-navigation Related to the app-navigation component labels Mar 5, 2024
@Pytal Pytal added this to the 8.8.2 milestone Mar 5, 2024
@Pytal Pytal self-assigned this Mar 5, 2024
@Pytal Pytal force-pushed the fix/app-nav-list-scroll branch from 34d43d7 to acb55e4 Compare March 5, 2024 14:05
Comment on lines 67 to 71
<div v-if="$scopedSlots.default" class="app-navigation__body">
<slot />
</div>

Copy link
Contributor

@ShGKme ShGKme Mar 5, 2024

Choose a reason for hiding this comment

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

With the default slot, it is always more complicated. If short syntax (not <template #default>) is used, then the slot is passed. Even it is actually empty with empty strings from the template.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

$slots will work here in Vue 2 as well as double-render with isSlotPopupated() in Vue 3

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the default slot, it is always more complicated. If short syntax (not <template #default>) is used, then the slot is passed. Even it is actually empty with empty strings from the template.

In this case it seems to not be an empty string but rather a modal https://github.com/nextcloud/server/blob/c18ffe0cad13dc5903cbb08ffbdb787f01db5e35/apps/files/src/views/Navigation.vue#L71-L73 that is causing this 🤔

Not sure how we could resolve this on nc/vue side, any ideas?

And because of flex, it is stretched by default:

How can I repro this?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case it seems to not be an empty string but rather a modal https://github.com/nextcloud/server/blob/c18ffe0cad13dc5903cbb08ffbdb787f01db5e35/apps/files/src/views/Navigation.vue#L71-L73 that is causing this 🤔

Yes, but the same issue would be in other cases. Anything in components's content, even if it actually is not rendered, cause this issue. With the default slot it's always more annoying...

Not sure how we could resolve this on nc/vue side, any ideas?

Let me have a look on the issue...

How can I repro this?

I though that was on this branch + your branch on server for the same issue. Be let me double check.

Copy link
Contributor

@ShGKme ShGKme Mar 5, 2024

Choose a reason for hiding this comment

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

And because of flex, it is stretched by default:

How can I repro this?

Ok, it seems to work fine now on your branch.

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.

seems to good ;) thank you!

@Pytal
Copy link
Contributor Author

Pytal commented Mar 6, 2024

Pushed a fixup (squashed!) to document NOT putting any NcModal/NcDialog inside the NcAppNavigation default slot

Only one place https://github.com/nextcloud/server/blob/c18ffe0cad13dc5903cbb08ffbdb787f01db5e35/apps/files/src/views/Navigation.vue#L71-L73 would need to be adjusted

@Pytal Pytal force-pushed the fix/app-nav-list-scroll branch from 88be19b to 43fe184 Compare March 6, 2024 11:51
@Pytal Pytal force-pushed the fix/app-nav-list-scroll branch from 43fe184 to 19e7f64 Compare March 6, 2024 11:55
@Pytal Pytal merged commit 27d880a into master Mar 6, 2024
18 checks passed
@Pytal Pytal deleted the fix/app-nav-list-scroll branch March 6, 2024 11:59
@juliusknorr juliusknorr mentioned this pull request Mar 6, 2024
@juliusknorr juliusknorr modified the milestones: 8.8.2, 8.9.0 Mar 6, 2024
@ShGKme
Copy link
Contributor

ShGKme commented Mar 6, 2024

Should be also tested with Talk

@ShGKme
Copy link
Contributor

ShGKme commented Mar 6, 2024

It breaks Talk
image

@ShGKme
Copy link
Contributor

ShGKme commented Mar 6, 2024

And mail. So, any app that has both default and list slots.

image

@Pytal
Copy link
Contributor Author

Pytal commented Mar 6, 2024

It breaks Talk

And mail. So, any app that has both default and list slots.

💀💀 we'll need to find a solution without using default slot then

I think then we should add a new slot for this specific use-case instead of changing default slot behaviour, what do you think @ShGKme?

@ShGKme
Copy link
Contributor

ShGKme commented Mar 6, 2024

I think then we should add a new slot for this specific use-case instead of changing default slot behaviour, what do you think @ShGKme?

I'd not add a new slot here. A new slot means — +4 cases to support in the component (combination with other slots) and a new interface — so no revert without a breaking change.

💀💀 we'll need to find a solution without using default slot then

I think, using the default slot is fine. The problem is probably mixing some styles between #list slot in <NcAppNavigation> and <NcAppNavigationList> which can be used in both the default slot and list.

We can try to keep #list slot styles as it is, removing them from the component itself, and then fix layout on the User settings page because it has it's own layout.

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.

5 participants