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(NcAppNavigationItem): Fix popper boundary element #5632

Merged

Conversation

ChristophWurst
Copy link
Contributor

@ChristophWurst ChristophWurst commented May 22, 2024

☑️ Resolves

The query selector in the computed prop ran too early, before the element existed. There is no reactivity. Now the boundary element is set very late at mount, when the boundary element does exist in the DOM.

There are a few edge cases, but the menu positions better than before when the boundary fell back to the body.

🖼️ Screenshots

🏚️ Before 🏡 After
B A

🚧 Tasks

  • ...

🏁 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

@ChristophWurst ChristophWurst self-assigned this May 22, 2024
@ChristophWurst ChristophWurst added bug Something isn't working 2. developing Work in progress feature: app-navigation Related to the app-navigation component feature: popover Related to the popovermenu component labels May 22, 2024
@GretaD
Copy link
Contributor

GretaD commented May 29, 2024

I end up in the same scenario as you. I can reproduce the issue on main, then i link and the issue is gone. I do npm ci, kill the link and same as you i cannot reproduce it anymore.

@ChristophWurst
Copy link
Contributor Author

Let's do this. It sets the element more reliably. I don't see a downside.

@ChristophWurst ChristophWurst marked this pull request as ready for review June 26, 2024 14:35
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 26, 2024
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Makes sense because on mounted the parent should exist which is the content.

And the computed was wrong in the first place, it was never recalculated because document is not reactive

@susnux
Copy link
Contributor

susnux commented Jun 27, 2024

/backport to next

@ChristophWurst ChristophWurst force-pushed the fix/ncappnavigationitem/popper-boundary-element branch from 43034e8 to a30de5b Compare July 8, 2024 08:07
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 8, 2024
@ChristophWurst ChristophWurst merged commit e258d13 into master Jul 8, 2024
20 checks passed
@ChristophWurst ChristophWurst deleted the fix/ncappnavigationitem/popper-boundary-element branch July 8, 2024 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug Something isn't working feature: app-navigation Related to the app-navigation component feature: popover Related to the popovermenu component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants