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

feat(legend): add keyboard navigation #880

Merged
merged 56 commits into from
Dec 9, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
8846dd2
feat: add tabbing into legend
rshen91 Oct 27, 2020
3f66d55
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Oct 27, 2020
b85af45
feat: add focus to navigation of legend
rshen91 Oct 28, 2020
5342cc8
style: fixing echLegendLlistContainer orientation
rshen91 Oct 29, 2020
5bfd8ab
fix: remove outer button fixing style issues
rshen91 Oct 30, 2020
7ded2df
test: update unit tests
rshen91 Oct 30, 2020
f783dba
fix: improve navigation for action story
rshen91 Nov 3, 2020
16fcfa7
feat: add screen reader for extra
rshen91 Nov 6, 2020
d0d4a2e
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 6, 2020
c4b60e1
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 6, 2020
cfd84a6
fix: change aria label and unit test snapshots
rshen91 Nov 9, 2020
8bafadd
test: update vrts
rshen91 Nov 9, 2020
09c1048
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 11, 2020
69c528f
refactor: add some cleanup
rshen91 Nov 11, 2020
835655f
test: revert vrt updates
rshen91 Nov 11, 2020
4637ccc
feat: initial review changes
rshen91 Nov 11, 2020
f7e2f27
test: update unit tests legend
rshen91 Nov 12, 2020
ec97027
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 12, 2020
5b842fe
style: fix focus styling
rshen91 Nov 12, 2020
7753767
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 12, 2020
bc14508
fix: add color name to aria label
rshen91 Nov 16, 2020
038f30c
fix: cleanup style and test
rshen91 Nov 16, 2020
e5a83e4
fix: add ownFocus to color story
rshen91 Nov 16, 2020
dc373e8
fix: add changes to label for isSeriesHidden
rshen91 Nov 16, 2020
cde7eb9
fix: add label to aria label
rshen91 Nov 16, 2020
07eadda
fix: add clearer aria label to color picker
rshen91 Nov 17, 2020
c78bfce
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 17, 2020
f751a99
style: incorporate style changes in review
rshen91 Nov 17, 2020
e2f516a
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 20, 2020
ef51a44
WIP
rshen91 Nov 23, 2020
a2cfae8
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 24, 2020
a85b704
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 25, 2020
5e29295
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 25, 2020
35425e2
test: add puppeteer test for aria label and tabbing on legend items
rshen91 Nov 30, 2020
0538d65
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Nov 30, 2020
27c4826
test: add async test for tab and enter
rshen91 Nov 30, 2020
58a74d4
test: update vrts
rshen91 Nov 30, 2020
326cc37
refactor: update async test
rshen91 Nov 30, 2020
07fd6e2
test: add style changes and revert vrts
rshen91 Dec 1, 2020
92eb0e2
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Dec 2, 2020
ecc7797
feat: update aria label for isSeriesHidden
rshen91 Dec 2, 2020
48f01e7
test: euiring styling vrt update on tests
rshen91 Dec 2, 2020
fc8c562
test: flaky test update and second wait for legend test
rshen91 Dec 2, 2020
dd19e9d
test: update vrts
rshen91 Dec 2, 2020
d14a03a
test: add style consistency to test
rshen91 Dec 2, 2020
03ee240
test: add style test for pupeteer
rshen91 Dec 3, 2020
c260426
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Dec 3, 2020
3631426
test: add disable-animations to common and area stories
rshen91 Dec 4, 2020
64e99f8
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Dec 4, 2020
02a2555
test: add missing area vrts
rshen91 Dec 4, 2020
5bdb611
refactor: remove url parameter from disableAnimations
rshen91 Dec 6, 2020
8932129
Merge remote-tracking branch 'upstream/master' into legend-keyboard
rshen91 Dec 7, 2020
55f1628
style: remove old comments
rshen91 Dec 9, 2020
02a06fe
style: disable eslint in lines vs entire file
rshen91 Dec 9, 2020
a5acd93
refactor: pr feedback changes
rshen91 Dec 9, 2020
547f69d
Merge branch 'master' into legend-keyboard
rshen91 Dec 9, 2020
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
23 changes: 12 additions & 11 deletions .storybook/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,18 @@ html {
box-sizing: border-box;
background-color: blanchedalmond;

// *,
// *::after,
// *::before,
// .echLegendItem {
// transition-delay: 0s !important;
// transition-duration: 0s !important;
// animation-delay: -0.0001s !important;
// animation-duration: 0s !important;
// animation-play-state: paused !important;
// caret-color: transparent !important;
// }
&.disable-animations {
*,
*::after,
*::before {
transition-delay: 0s !important;
transition-duration: 0s !important;
animation-delay: -0.0001s !important;
animation-duration: 0s !important;
animation-play-state: paused !important;
caret-color: transparent !important;
}
}
}

#story-root + div table {
Expand Down
1 change: 0 additions & 1 deletion integration/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,4 @@ module.exports = {
PORT: '9009',
HOST: process.env.DEBUG === 'true' ? 'localhost' : 'host.docker.internal',
LOCAL_STORYBOOK_VRT: false,
global_setup: '../../.storybook/style_test.scss',
};
29 changes: 8 additions & 21 deletions integration/page_objects/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -347,26 +347,6 @@ class CommonPage {
* @param options
*/
async expectChartAtUrlToMatchScreenshot(url: string, options?: ScreenshotElementAtUrlOptions) {
await page.evaluateOnNewDocument(() => {
const style = document.createElement('style');
style.type = 'text/css';
style.innerHTML = `
*,
*::after,
*::before,
.echLegendItem {
transition-delay: 0s !important;
transition-duration: 0s !important;
animation-delay: -0.0001s !important;
animation-duration: 0s !important;
animation-play-state: paused !important;
caret-color: transparent !important;
};`;
const storyRoot = document.getElementById('#story-root');
if (storyRoot) {
storyRoot.appendChild(style);
}
});
await this.expectElementAtUrlToMatchScreenshot(url, this.chartSelector, {
waitSelector: this.chartWaitSelector,
...options,
Expand Down Expand Up @@ -404,8 +384,8 @@ class CommonPage {
options?: Omit<ScreenshotElementAtUrlOptions, 'action'>,
) {
const action = async () => {
await this.disableAnimations('http://localhost:9001/?path=/story/legend--right');
await this.clickMouseRelativeToDOMElement({ top: 242, left: 910 }, this.chartSelector);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a generic method, why are there specific top and left values? Is the purpose of this to just click the element to the tap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Just to make sure that storybook tabbing doesn't go down all the individual stories in the pane before reaching the iframe


// eslint-disable-next-line no-restricted-syntax
for (const actions of keyboardEvents) {
await this.pressKey(actions.actionLabel, actions.count);
Expand Down Expand Up @@ -460,6 +440,13 @@ class CommonPage {
});
}

async disableAnimations(url: string) {
await this.loadElementFromURL(url, '#story-root');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good thought. It must replace the #story-root after each navigation.

But as long as you use this in the action function you don't need to call loadElementFromURL.

Suggested change
await this.loadElementFromURL(url, '#story-root');

await page.evaluate(() => {
document.querySelector('#story-root')!.classList.add('disable-animations');
});
}

/**
* Wait for an element to be on the DOM
*
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
22 changes: 16 additions & 6 deletions integration/tests/area_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,27 +64,37 @@ describe('Area series stories', () => {
});
});
describe('Negative log Areas', () => {
it('snows negative values with log scale', async () => {
it('shows negative values with log scale', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/area-chart--with-negative-values&knob-Y scale=log',
);
});
it('snows only positive domain mixed polarity domain', async () => {
it('shows only positive domain mixed polarity domain', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',
);
});

it('snows only positive values when hiding negative one', async () => {
const action = async () => await page.click('.echLegendItem:nth-child(2) .echLegendItem__label');
it('shows only positive values when hiding negative one', async () => {
const action = async () => {
await common.disableAnimations(
'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to repeat the URL navigation again. The disableAnimations method can just be used as a helper functions in the action.

Suggested change
await common.disableAnimations(
'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',
);
await common.disableAnimations();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉 Good point - please see 5bdb611

await page.click('.echLegendItem:nth-child(2) .echLegendItem__label');
};
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',
{ action },
);
});

it('snows only negative values when hiding positive one', async () => {
const action = async () => await page.click('.echLegendItem:nth-child(1) .echLegendItem__label');
it('shows only negative values when hiding positive one', async () => {
const action = async () => {
await common.disableAnimations(
'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',
);
Copy link
Collaborator

@nickofthyme nickofthyme Dec 5, 2020

Choose a reason for hiding this comment

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

Same as above. Lmk if this doesn't work.

Suggested change
await common.disableAnimations(
'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',
);
await common.disableAnimations();

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 works! Thank you! Please see 5bdb611

await page.click('.echLegendItem:nth-child(1) .echLegendItem__label');
};
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',
{ action },
Expand Down
3 changes: 3 additions & 0 deletions integration/tests/legend_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ describe('Legend stories', () => {
});
});
describe('keyboard navigation', () => {
beforeEach(async () => {
await common.disableAnimations('http://localhost:9001/?path=/story/legend--right');
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - totally not needed - removed in 5bdb611

// eslint-disable-next-line jest/expect-expect
it('should navigate to legend item with tab', async () => {
await common.expectChartWithKeyboardEventsAtUrlToMatchScreenshot(
Expand Down