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

Remove explicit use of aria-hidden for offcanvas when closed #35589

Merged
merged 11 commits into from
Jan 5, 2022

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Dec 23, 2021

due to the .offcanvas styles already containing visibility:hidden, no need to add aria-hidden nor to explicitly set the style="" attribute when the offcanvas gets closed

Closes #35577

@XhmikosR
Copy link
Member

Can you remove the dist files please?

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Looks like you commited dist files. We can take care of it if needed 👌

Edit: Oh, just saw previous comment!

due to the styles `.offcanvas` styles already containing `visibility:hidden`, no need to add `aria-hidden` nor to explicitly set the `style=""` attribute when the offcanvas gets closed
@patrickhlauke patrickhlauke force-pushed the patrickhlauke-simplify-offcanvas branch from 40f97e9 to 57de860 Compare December 23, 2021 09:34
@patrickhlauke
Copy link
Member Author

urgh, stupid dist files...

@XhmikosR XhmikosR requested review from ffoodd and GeoSot December 23, 2021 12:43
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@GeoSot
Copy link
Member

GeoSot commented Dec 24, 2021

@patrickhlauke it will need something more (maybe one more class like .transitioning) in order to achieve better transition during close.

Ps: I can help if you like 😃

@patrickhlauke
Copy link
Member Author

@patrickhlauke it will need something more (maybe one more class like .transitioning) in order to achieve better transition during close.

Aha, that explains it. I was wondering why all of a sudden this was setting/removing actual style attributes separately rather than just relying on classes. Clearly I'm a bit rusty (and hadn't fully checked the impact visually). Yeah if you don't mind giving an assist, that'd be awesome.

@patrickhlauke
Copy link
Member Author

Let me set this to draft in the meantime so it doesn't get accidentally merged prematurely

@patrickhlauke patrickhlauke marked this pull request as draft December 24, 2021 00:27
@GeoSot
Copy link
Member

GeoSot commented Dec 24, 2021

Please check the functionality & changes, and if we agree on these, I will 'tweak' the test

Just to note down, that this approach (using more classes), would help in the near future streamlining most of the js components

@patrickhlauke
Copy link
Member Author

@GeoSot just tested, seems to work really well. If you don't mind tweaking the tests, that'd be great

@patrickhlauke patrickhlauke marked this pull request as ready for review December 25, 2021 20:54
@patrickhlauke patrickhlauke requested a review from ffoodd December 25, 2021 20:54
@GeoSot
Copy link
Member

GeoSot commented Jan 5, 2022

@ffoodd I would review it but I have also put my hands in here, so your review would help us 💯

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

I better aligned the CSS with our current code style. This is working fine so :shipit:

@GeoSot
Copy link
Member

GeoSot commented Jan 5, 2022

I better aligned the CSS with our current code style. This is working fine so

@ffoodd forgot the & and now it scopes a child element 😉

@ffoodd
Copy link
Member

ffoodd commented Jan 5, 2022

Oh my, thanks :D

@GeoSot
Copy link
Member

GeoSot commented Jan 5, 2022

It's ALIVE again :D

@GeoSot GeoSot merged commit 0d054bb into main Jan 5, 2022
@GeoSot GeoSot deleted the patrickhlauke-simplify-offcanvas branch January 5, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offcanvas navbar expand remaining aria-hidden
4 participants