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): manage focus only after transition has finished #5833

Merged

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jul 18, 2024

☑️ Resolves

When NcAppSidebar appears, two things happen:

  1. Transition animation with margin
  2. Focus is moved to the sidebar

Moving focus scrolls container (NcContent) to the sidebar which is out of the container (in the process of moving from the right side). It results in weird animation when the margin changes the width + scroll of the container instead of the position of the sidebar.

P.S. overflow: hidden only hides the scrollbar and content, but the container is still scrollable.
overflow: clip is used to actually remove scrolling.

Slowed animation 1000 times to see the effect: sidebar moves left and right by 1 pixel.

slow

🖼️ Screenshots

🏚️ Before 🏡 After
before after

🚧 Tasks

  • Move focus management to transition hooks
    • Both return focus and focus trap

Alternative solution

Use overflow: clip on NcContent to remove scrolling.

🏁 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 3. to review Waiting for reviews feature: app-sidebar Related to the app-sidebar component labels Jul 18, 2024
@ShGKme ShGKme added this to the 8.14.1 milestone Jul 18, 2024
@ShGKme ShGKme requested review from susnux and Antreesy July 18, 2024 19:05
@ShGKme ShGKme self-assigned this Jul 18, 2024
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/nc-app-sidebar--manage-focus-only-after-transition-finished branch from 4c45bf0 to ce944c6 Compare July 18, 2024 19:09
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.

Tested

@ShGKme ShGKme merged commit 5e21eb5 into master Jul 19, 2024
19 checks passed
@ShGKme ShGKme deleted the fix/nc-app-sidebar--manage-focus-only-after-transition-finished branch July 19, 2024 15:31
@ShGKme
Copy link
Contributor Author

ShGKme commented Jul 19, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching to a chat from the root page has a UI glitch if sidebar was open before
3 participants