-
Notifications
You must be signed in to change notification settings - Fork 163
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: remove collapsed side panel from accessibility tree #4628
fix: remove collapsed side panel from accessibility tree #4628
Conversation
Demo starting at https://vanilla-framework-4628.demos.haus |
@@ -235,7 +235,7 @@ $application-layout--side-nav-width-expanded: 15rem !default; | |||
} | |||
|
|||
.l-aside { | |||
@include vf-transition($property: #{transform, box-shadow}, $duration: snap); | |||
@include vf-transition($property: #{transform, box-shadow, visibility}, $duration: snap); |
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.
Is visibility a prop that can be transitioned?
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.
Try the demo with visibility transition and without to see for yourself 馃槃
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.
Cool! That simplifies a lot of things.
Based on the description it seems it "should" switch to visibility: hidden as soon as animated value is closer to 0 (so less than 0.5?) which would mean it disapears mid-transition. This doesn't seem to happen (which is good).
Anyway, thanks for the fix! We need to look if there are any other places that would benefit from that. I recall us implementing some workarounds for similar issues in side navigation.
@petermakowski Could you bump version in package.json to 3.10.1? Thanks! |
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.
Just bump the patch version in package.json and it's good to merge, thanks!
Done
Fixes: #4629
QA
Check if PR is ready for release
If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:
Feature 馃巵
,Breaking Change 馃挘
,Bug 馃悰
,Documentation 馃摑
,Maintenance 馃敤
.package.json
should be updated relative to the most recent release, following semver convention:Screenshots
[if relevant, include a screenshot or screen capture]