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

Add aria-hidden attr to the whole navigation depending on a sidebar state #4016

Conversation

JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Apr 28, 2023

Comment on lines +198 to +193
display: flex;
flex-direction: column;
flex-grow: 0;
flex-shrink: 0;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@ShGKme ShGKme left a 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>
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/36926-The_visually_hidden_elements_of_the_navigation_on_the_left_are_always_read_by_screen_readers branch from 30a0eea to 1c08751 Compare May 3, 2023 13:55
@artonge artonge merged commit 75cde38 into master May 3, 2023
@artonge artonge deleted the fix/36926-The_visually_hidden_elements_of_the_navigation_on_the_left_are_always_read_by_screen_readers branch May 3, 2023 14:16
@artonge artonge mentioned this pull request May 3, 2023
Comment on lines -159 to -162
display: flex;
flex-direction: column;
flex-grow: 0;
flex-shrink: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs reverting it heavily breaks the talk left sidebar:
grafik

Copy link
Contributor

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

Copy link
Contributor

@skjnldsv skjnldsv May 4, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor

@ShGKme ShGKme May 4, 2023

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;

Copy link
Contributor

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.

Copy link
Contributor Author

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!

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
Projects
None yet
6 participants