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.

Sign up for GitHub

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

fix(ThumbnailsSidebar): A11Y - add aria-current to ThumbnailsSidebar #1417

Merged
merged 7 commits into from
Aug 10, 2021
Merged

fix(ThumbnailsSidebar): A11Y - add aria-current to ThumbnailsSidebar #1417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
05da910
fix: A11Y - add aria-current to ThumbnailsSidebar
fsuarez-globant Aug 5, 2021
d043fa1
fix: A11Y - add aria-current to ThumbnailsSidebar
fsuarez-globant Aug 5, 2021
811c086
fix: A11Y - add aria-current to ThumbnailsSidebar
fsuarez-globant Aug 5, 2021
01d7837
fix: A11Y - add unit tests
fsuarez-globant Aug 6, 2021
acdcf2c
fix: A11Y - add unit tests
fsuarez-globant Aug 6, 2021
057a8e2
fix: A11Y - add unit tests
fsuarez-globant Aug 6, 2021
b492c7c
fix: A11Y - add unit tests
fsuarez-globant Aug 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/lib/ThumbnailsSidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ class ThumbnailsSidebar {

if (pageNum === this.currentPage) {
thumbnailEl.classList.add(CLASS_BOX_PREVIEW_THUMBNAIL_IS_SELECTED);
thumbnailEl.setAttribute('aria-current', true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this value get updated as the user changes between pages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jstoffan you are right. I added and removed the aria-current when user changes between pages.

}

// If image is already in cache, then use it instead of waiting for
Expand Down Expand Up @@ -439,8 +440,10 @@ class ThumbnailsSidebar {
const parsedPageNum = parseInt(thumbnailEl.dataset.bpPageNum, 10);
if (parsedPageNum === this.currentPage) {
thumbnailEl.classList.add(CLASS_BOX_PREVIEW_THUMBNAIL_IS_SELECTED);
thumbnailEl.setAttribute('aria-current', true);
} else {
thumbnailEl.classList.remove(CLASS_BOX_PREVIEW_THUMBNAIL_IS_SELECTED);
thumbnailEl.removeAttribute('aria-current');
}
});
}
Expand Down
20 changes: 20 additions & 0 deletions src/lib/__tests__/ThumbnailsSidebar-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,13 +439,17 @@ describe('ThumbnailsSidebar', () => {
beforeEach(() => {
stubs.addClass = jest.fn();
stubs.removeClass = jest.fn();
stubs.addAriaAttribute = jest.fn();
stubs.removeAriaAttribute = jest.fn();

// eslint-disable-next-line
const createTestThumbnail = pageNum => {
const thumbnail = document.createElement('div');
thumbnail.dataset.bpPageNum = pageNum;
thumbnail.classList.add = stubs.addClass;
thumbnail.classList.remove = stubs.removeClass;
thumbnail.setAttribute = stubs.addAriaAttribute;
thumbnail.removeAttribute = stubs.removeAriaAttribute;
return thumbnail;
};

Expand Down Expand Up @@ -475,6 +479,22 @@ describe('ThumbnailsSidebar', () => {
expect(stubs.removeClass).toBeCalledTimes(2);
expect(stubs.addClass).toBeCalledTimes(1);
});

test('should add the aria-current attribute when thumbnail is selected', () => {
thumbnailsSidebar.currentPage = 2;

thumbnailsSidebar.applyCurrentPageSelection();

expect(stubs.addAriaAttribute).toBeCalledTimes(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add an assertion for the removeAttribute call, as well, similar to the tests above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jstoffan I just added another unit test case.

});

test('should remove the aria-current attribute when thumbnail is not selected', () => {
thumbnailsSidebar.currentPage = 2;

thumbnailsSidebar.applyCurrentPageSelection();

expect(stubs.removeAriaAttribute).toBeCalledTimes(2);
});
});

describe('toggle()', () => {
Expand Down