-
Notifications
You must be signed in to change notification settings - Fork 91
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
Add aria-hidden attr to the whole navigation depending on a sidebar state #4016
Add aria-hidden attr to the whole navigation depending on a sidebar state #4016
Conversation
display: flex; | ||
flex-direction: column; | ||
flex-grow: 0; | ||
flex-shrink: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These styles are moved from .app-navigation
, which previously was the container of slots and the list, to the new container. Could we now remove them from the .app-navigation
or they are still required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is hard to test all apps and the risk to break something is high. I could remove https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/NcAppNavigation/NcAppNavigation.vue#L161-L162 but would like leave it to be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐸
…tate Signed-off-by: julia.kirschenheuter <julia.kirschenheuter@nextcloud.com>
30a0eea
to
1c08751
Compare
display: flex; | ||
flex-direction: column; | ||
flex-grow: 0; | ||
flex-shrink: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I would have blocked if seen on time, this seems absolutely not related to this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but not all of them.
These styles are flex container styles. They should have been moved.
display: flex;
flex-direction: column;
while these are flex elements styles. They should be only here.
flex-grow: 0;
flex-shrink: 0;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems absolutely not related to this PR
50% of these styles are flex container styles. In this PR there was created a new flex container for all content, and all flex container styles were moved to the new flex container. So, they are related.
Although, another 50% were flex element styles and really not related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this what i feared: it is hard to test every app. Thank you @ShGKme for explanation and fixing!
Fix an old sidebar navigation on server.
This fix is only for (where
app-navigation-vue
is used):Summary
The goal of this fix: don't allow screen reader navigation through a sidebar if sidebar is closed. No visual changes.
After: