-
Notifications
You must be signed in to change notification settings - Fork 601
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
Feature/watch visibility #1032
base: master
Are you sure you want to change the base?
Feature/watch visibility #1032
Conversation
Thanks so much for this contribution. This has been requested several times. I'll be evaluating adding this feature in the next minor release. |
@desandro, great! And when will that be? |
@larscmagnusson Could this PR be updated to also properly add/remove the aria-hidden attribute based on the cell visibility? |
@desandro once this PR is updated to handle the aria-hidden could it be merged? If a given slide with aria-hidden=true contains focusable elements, Lighthouse now returns this as an accessibility issue. A simple fix would be to set aria-hidden elements to visibility: hidden, but the fact that Flickity currently does not handle the aria-hidden attribute correctly prevent us to do that :( |
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.
The PR #1032 works great! Please change the following to flickity.js on line 766 to also support aria-hidden on visible slides for PR #1015 (:
if (isVisible) {
cell.element.classList.add('is-visible');
cell.element.removeAttribute('aria-hidden');
} else {
cell.element.classList.remove('is-visible');
cell.element.setAttribute('aria-hidden', true);
}
@jenswittmann Done |
|
||
this.cells.forEach(function (cell) { | ||
var cellX = cell.element.getBoundingClientRect().x - viewportX; | ||
var isVisible = ( |
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.
In my testing, isVisible
will be false when the is-selected
cell is itself wider than the browser window, despite that the cell is obviously visible (being the is-selected
cell centered on screen).
|
||
this.cells.forEach(function (cell) { | ||
var cellX = cell.element.getBoundingClientRect().x - viewportX; | ||
var isVisible = ( |
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.
isVisible
will not function correctly unless the slider is full width. Anything in-line will not work if the cell is not wider than the viewport offset.
I have added a test to check if the value of cellX === 0
as it is the first element within the viewport if the value of cellX is 0 (More specifically I am checking if cellX > -1 && cellX < 1
due to the fluctuation of cell placement when the carousel stops moving).
This is not an ideal solution as I am getting flashing before a slide comes to rest, but it is solving the issue in my use case.
I would love to see this issue resolved. |
Any updates on this? |
Is there any update on this? |
Can we merge this please? Would love to have this issue fixed! |
What is the status on this issue? We are using Flickity and are getting flagged for accessibility. |
This feature adds or removes the className
is-visible
based on cells' condition.