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(NcAppSidebar): make sidebar a single node again to allow v-show, classes and attributes #5627

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented May 21, 2024

☑️ Resolves

Makes <NcAppSidebar> rendered as a single node again.

Example of broken apps - Files:

  • No cy-data-sidebar attributes for e2e
  • No app-sidebar--full class
  • No data-v-{scopeId} attr required for app-sidebar--full

Talk:

  • Sidebar is always shown

🖼️ Screenshots

🏚️ Before 🏡 After
image image
image image

🚧 Tasks

  • Make <NcAppSidebar> rendered as a single node again
  • Render the toggle button using teleport

🏁 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

@ShGKme ShGKme added bug Something isn't working 2. developing Work in progress regression Regression of a previous working feature feature: app-sidebar Related to the app-sidebar component labels May 21, 2024
@ShGKme ShGKme added this to the 8.12.1 milestone May 21, 2024
@ShGKme ShGKme self-assigned this May 21, 2024
@ShGKme ShGKme force-pushed the fix/NcAppSidebar--use-single-root-element branch 2 times, most recently from 893fe47 to 5a12f6b Compare May 21, 2024 16:03
@susnux
Copy link
Contributor

susnux commented May 21, 2024

I vote for not backporting this to next and instead require to use open prop. Maybe introduce no-toggle prop to hide the toggle button.

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.

Do you prefer this one? Because I think it adds a lot of complexity.
If yes I will free some time for a review 😅

@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 13, 2024

Do you prefer this one?

I dislike both.

I think this is much simpler (check diff with hide whitespaces and without looking into docs — it's literally just removing <Fragment> and adding <Portal>).

But this could be more risky because we need to rely on #content-vue or another selector and because Teleport to a node inside Virtual DOM is always risky.

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.

works.

I am fine with both solutions 🤷

@ShGKme ShGKme force-pushed the fix/NcAppSidebar--use-single-root-element branch from e8c9d4a to bc7cc6d Compare June 14, 2024 09:40
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 14, 2024

  • Rebased onto master to resolve conflicts
  • Added a small fixup:
    • Removed inheritAttrs: false (not needed anymore)
    • Moved v-if from <NcButton> to <teleport> to not have unneeded teleport without button.

@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 14, 2024
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

I'd personnaly prefer this solution (not a fan of several nodes under root template, now it's more predictable).
Tested, works fine as a middleground before moving to open.sync

@ShGKme ShGKme force-pushed the fix/NcAppSidebar--use-single-root-element branch from bc7cc6d to bd32d5d Compare June 19, 2024 18:14
@ShGKme ShGKme modified the milestones: 8.12.1, 8.13.0 Jun 19, 2024
@ShGKme ShGKme changed the title fix(NcAppSidebar): make sidebar a single node to allow using v-show fix(NcAppSidebar): make sidebar a single node again to allow v-show, classes and attributes Jun 19, 2024
@ShGKme ShGKme marked this pull request as ready for review June 19, 2024 18:16
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 19, 2024

Let's then go with this solution. While teleport is a bit risky, it allows making it a real single root component without hacks and drawbacks.

If we find a problem with Teleport, then we can proceed with the alternative solution.

I've added a check to make sure that it is used in NcContent when open prop is used. Maybe overhead.

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/NcAppSidebar--use-single-root-element branch from bd32d5d to 612fa75 Compare June 20, 2024 10:00
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 20, 2024

Rebased onto master and squashed

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme merged commit ad85850 into master Jun 20, 2024
19 checks passed
@ShGKme ShGKme deleted the fix/NcAppSidebar--use-single-root-element branch June 20, 2024 14:00
@susnux
Copy link
Contributor

susnux commented Jun 20, 2024

@ShGKme backport?

@susnux susnux mentioned this pull request Jun 20, 2024
1 task
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 20, 2024

/backport to next

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-sidebar Related to the app-sidebar component regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NcAppSidebar] v-show doesn't work on sidebar anymore because of Fragment
3 participants