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): add focus trap on mobile #4909

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Dec 4, 2023

☑️ Resolves

On mobile view, the sidebar is full-screen. But it doesn't have a focus trap. So the focus may fall into the main content below the sidebar.

🖼️ Screenshots

🏚️ Before 🏡 After
Focus is lost on the main content It's a trap
sidebar-before sidebar-after

🚧 Tasks

  • Add a focus trap on mobile
  • Add a header to the trap as it is fully visible

For discussion

  • Add a navigation toggle button to the trap so a user can open navigation above the focus trap
    • Alternative — hide toggle navigation button on mobile
  • Close the sidebar by ESC on mobile

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme ShGKme added bug Something isn't working 2. developing Work in progress feature: app-sidebar Related to the app-sidebar component accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Dec 4, 2023
@ShGKme ShGKme added this to the 8.3.1 milestone Dec 4, 2023
@ShGKme ShGKme self-assigned this Dec 4, 2023
@ShGKme ShGKme force-pushed the fix/sidebar-focus-trap-on-mobile branch from 800b8bc to 8415a2b Compare December 4, 2023 15:44
@ShGKme
Copy link
Contributor Author

ShGKme commented Dec 4, 2023

There is an issue with NcActions on mobile view now. But the same issue happens in NcAppNavigation, NcModal, and other components with the focus trap.

See: #4910

@ShGKme ShGKme force-pushed the fix/sidebar-focus-trap-on-mobile branch from 8415a2b to c118ae3 Compare December 5, 2023 16:19
@ShGKme ShGKme mentioned this pull request Dec 5, 2023
2 tasks
@ShGKme ShGKme force-pushed the fix/sidebar-focus-trap-on-mobile branch 3 times, most recently from a004cad to af1ba1f Compare December 6, 2023 15:28
@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 6, 2023
@ShGKme ShGKme force-pushed the fix/sidebar-focus-trap-on-mobile branch 2 times, most recently from 8433b64 to 5e8aa01 Compare December 7, 2023 10:51
@ShGKme ShGKme marked this pull request as ready for review December 7, 2023 15:59
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.

Works, great!

do we need a focus here

tabindex="0"
@click="onFigureClick"
?

@ShGKme
Copy link
Contributor Author

ShGKme commented Dec 7, 2023

Works, great!

do we need a focus here

tabindex="0"
@click="onFigureClick"

?

Yep, but it should also have a button role and have a label

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Seems like an important improvement! 👍

@ShGKme ShGKme force-pushed the fix/sidebar-focus-trap-on-mobile branch from 5e8aa01 to 66d0264 Compare December 11, 2023 16:35
@ShGKme
Copy link
Contributor Author

ShGKme commented Dec 11, 2023

Rebased onto master

@ShGKme ShGKme merged commit dd79bf8 into master Dec 11, 2023
15 checks passed
@ShGKme ShGKme deleted the fix/sidebar-focus-trap-on-mobile branch December 11, 2023 16:38
@ShGKme ShGKme mentioned this pull request Dec 22, 2023
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 bug Something isn't working feature: app-sidebar Related to the app-sidebar component
Projects
None yet
4 participants