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.

Sign up for GitHub

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

Jump to bottom

feat(NcAppSidebar): Allow to set open state to prevent focus trap issues on mobile #5584

Merged
merged 2 commits into from
May 17, 2024
Merged

feat(NcAppSidebar): Allow to set open state to prevent focus trap issues on mobile #5584

merged 2 commits into from
May 17, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented May 11, 2024 •
edited
Loading

☑️ Resolves

If the current screen size is "mobile" (<= 512px) and the app sidebar is not removed from dom but only hidden (v-show instead of v-if) then currently a user can not interact with the page anymore.
This happens because a focus trap is registered but not removed when the element is hidden.

So this adds a comment about using v-if but for performance (e.g the sidebar is hidden and shown often) the sidebar also now allows to set the open prop for hiding it.

🏁 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

@susnux susnux added bug Something isn't working 3. to review Waiting for reviews feature: app-sidebar Related to the app-sidebar component labels May 11, 2024
@susnux
Copy link
Contributor Author

susnux commented May 11, 2024

BTW the styleguide still does not work because multiple sidebars are opened that always set a focus trap.
So the only solution to fix the styleguide I can currently think of is hiding all examples by default.

@susnux susnux force-pushed the fix/nc-sidebar-open-state branch from 778139d to 2618252 Compare May 11, 2024 15:50
@susnux susnux mentioned this pull request May 11, 2024
Open
@ShGKme
Copy link
Contributor

ShGKme commented May 11, 2024

BTW the styleguide still does not work because multiple sidebars are opened that always set a focus trap. So the only solution to fix the styleguide I can currently think of is hiding all examples by default.

You mean, if styleguidist is open on mobile?

@susnux
Copy link
Contributor Author

susnux commented May 13, 2024

You mean, if styleguidist is open on mobile?

Yes, because currently one focus trap for each example is created and you can not interact with the page except for the first app sidebar example.

@susnux susnux force-pushed the fix/nc-sidebar-open-state branch from 2618252 to cf4f2bf Compare May 13, 2024 07:24
@susnux susnux requested a review from ShGKme May 13, 2024 07:24
@ShGKme ShGKme mentioned this pull request May 13, 2024
Merged
@susnux susnux force-pushed the fix/nc-sidebar-open-state branch from cf4f2bf to 4ecad73 Compare May 13, 2024 14:03
@susnux susnux requested a review from ShGKme May 13, 2024 14:04
@susnux
Copy link
Contributor Author

susnux commented May 13, 2024

@nextcloud-libraries/designers about the toggle icon, we have this discussion here:
#2804

So I used the icon recommended by @nimishavijay but got some feedback that it does not self descriptive for the sidebar.

Screenshot_20240513_161641

Maybe instead use menu-open? https://pictogrammers.com/library/mdi/icon/menu-open/

@susnux susnux requested a review from a team May 13, 2024 14:18
@susnux susnux force-pushed the fix/nc-sidebar-open-state branch from 4ecad73 to 2628763 Compare May 13, 2024 14:32
jancborchardt
jancborchardt requested changes May 14, 2024
View reviewed changes
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.

To make sure that it’s obvious and also different from the left navigation which uses menu-open, let’s use dock-right: https://pictogrammers.com/library/mdi/icon/dock-right/
dock-right

@susnux susnux force-pushed the fix/nc-sidebar-open-state branch from 2628763 to f2dd294 Compare May 16, 2024 08:30
@susnux
Copy link
Contributor Author

susnux commented May 16, 2024

@jancborchardt implemented ✔️

@susnux susnux requested a review from jancborchardt May 16, 2024 08:30
@susnux susnux force-pushed the fix/nc-sidebar-open-state branch 2 times, most recently from 8a9f744 to b0c5390 Compare May 16, 2024 10:22
@susnux susnux requested a review from ShGKme May 16, 2024 10:54
@ShGKme
Copy link
Contributor

ShGKme commented May 16, 2024

Checking attrs inheritance

ShGKme
ShGKme previously requested changes May 16, 2024
View reviewed changes
src/components/NcAppSidebar/NcAppSidebar.vue Outdated Show resolved Hide resolved
@susnux susnux force-pushed the fix/nc-sidebar-open-state branch 2 times, most recently from 4aeff83 to 91bb18a Compare May 16, 2024 15:43
@susnux susnux requested a review from ShGKme May 16, 2024 15:44
ShGKme
ShGKme approved these changes May 17, 2024
View reviewed changes
src/components/NcAppSidebar/NcAppSidebar.vue Outdated Show resolved Hide resolved
@ShGKme ShGKme mentioned this pull request May 17, 2024
Merged
susnux and others added 2 commits May 17, 2024 10:53
@susnux
feat(NcAppSidebar): Allow to set open state to prevent focus trap i…
5b52db6 
…ssues on mobile

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux @ShGKme
feat(NcAppSidebar): Add open toggle button
35c96b5 
To allow adjusting the position of the button a new `toggleClasses` prop is added.
This can be use to assign custom classes to be used with the `:deep` selector.

Co-authored-by: Ferdinand Thiessen <[email protected]>
Co-authored-by: Grigorii K. Shartsev <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/nc-sidebar-open-state branch from 9c65000 to 35c96b5 Compare May 17, 2024 08:53
@susnux susnux merged commit d4d2ee3 into master May 17, 2024
19 checks passed
@susnux susnux deleted the fix/nc-sidebar-open-state branch May 17, 2024 08:57
@susnux susnux mentioned this pull request May 17, 2024
Merged
susnux added a commit that referenced this pull request May 17, 2024
@susnux
Merge pull request #5606 from nextcloud-libraries/feat/backport-app-s…
7daf589 
…idebar

[next]  feat(NcAppSidebar): Allow to set open state to prevent focus trap issues on mobile #5584
@ShGKme ShGKme added enhancement New feature or request and removed bug Something isn't working labels May 19, 2024
@ShGKme ShGKme mentioned this pull request May 20, 2024
Closed
st3iny added a commit to nextcloud/calendar that referenced this pull request May 29, 2024
@st3iny
fix(editor): sidebar editor custom header styling
aec306b 
CSS styling overrides have to be adjusted after upstream changes.
Ref nextcloud-libraries/nextcloud-vue#5584

Signed-off-by: Richard Steinmetz <[email protected]>
@dartcafe dartcafe mentioned this pull request Jun 10, 2024
Closed
12 tasks
This was referenced Jun 11, 2024
Closed
Merged
@susnux susnux mentioned this pull request Jul 3, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShGKme ShGKme ShGKme approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@artonge artonge Awaiting requested review from artonge

@Pytal Pytal Awaiting requested review from Pytal

@Antreesy Antreesy Awaiting requested review from Antreesy

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement New feature or request feature: app-sidebar Related to the app-sidebar component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Form input fields not working at screen width less than 512 px [NcAppSidebar] add open state
3 participants
@susnux @ShGKme @jancborchardt