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 25 commits
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
59 changes: 10 additions & 49 deletions .playground/playground.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,67 +38,28 @@

import React from 'react';

import {
AnnotationDomainTypes,
Axis,
BarSeries,
Chart,
LineAnnotation,
LineAnnotationDatum,
ScaleType,
Settings,
} from '../src';
import { Icon } from '../src/components/icons/icon';
import { Axis, BarSeries, Chart, ScaleType, Settings } from '../src';
import { Position } from '../src/utils/commons';
import { arrayKnobs } from '../stories/utils/knobs';

function generateAnnotationData(values: any[]): LineAnnotationDatum[] {
return values.map((value, index) => ({ dataValue: value, details: `detail-${index}` }));
}
const data = arrayKnobs('data values', [2.5, 7.2]);
const dataValues = generateAnnotationData(data);
import * as TestDatasets from '../src/utils/data_samples/test_dataset';

export class Playground extends React.Component {
render() {
return (
<div className="chart">
<Chart className="story-chart">
<Settings showLegend showLegendExtra />
<LineAnnotation
id="annotation_1"
domainType={AnnotationDomainTypes.XDomain}
dataValues={dataValues}
marker={<Icon type="alert" />}
/>
<LineAnnotation id="1" domainType={AnnotationDomainTypes.YDomain} dataValues={dataValues} />
<Axis id="horizontal" position={Position.Bottom} title="x-domain axis" />
<Axis id="left" title="y-domain axis left" position={Position.Left} />
<Axis id="right" title="y-domain axis right" position={Position.Right} />
<Settings showLegend showLegendExtra legendPosition={Position.Right} />
<Axis id="bottom" position={Position.Bottom} title="Bottom axis" showOverlappingTicks />
<Axis id="left2" title="Left axis" position={Position.Left} tickFormat={(d) => Number(d).toFixed(2)} />

<BarSeries
id="bars"
groupId="group1"
xScaleType={ScaleType.Linear}
yScaleType={ScaleType.Linear}
xAccessor="x"
yAccessors={['y']}
data={[
{ x: 0, y: 0 },
{ x: 1, y: 5 },
{ x: 3, y: 20 },
]}
/>
<BarSeries
id="bars1"
groupId="group2"
xScaleType={ScaleType.Linear}
yScaleType={ScaleType.Linear}
xAccessor="x"
yAccessors={['y']}
data={[
{ x: 0, y: 100 },
{ x: 1, y: 50 },
{ x: 3, y: 200 },
]}
yAccessors={['y1', 'y2']}
splitSeriesAccessors={['g1', 'g2']}
data={TestDatasets.BARCHART_2Y2G}
hideInLegend={false}
/>
</Chart>
</div>
Expand Down
2 changes: 1 addition & 1 deletion src/components/__snapshots__/chart.test.tsx.snap
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Chart should render the legend name test 1`] = `"<div class=\\"echChart\\" style=\\"width: 100px; height: 100px;\\"><div class=\\"echChartBackground\\" style=\\"background-color: transparent;\\"></div><div class=\\"echChartStatus\\" data-ech-render-complete=\\"true\\" data-ech-render-count=\\"1\\"></div><div class=\\"echChartResizer\\"></div><div class=\\"echLegend echLegend--right echLegend--debug\\"><div style=\\"width: 50px; max-width: 50px; margin-left: 0px; margin-right: 0px;\\" class=\\"echLegendListContainer\\"><ul style=\\"padding-top: 10px; padding-bottom: 10px;\\" class=\\"echLegendList\\"><li class=\\"echLegendItem echLegendItem--right\\" data-ech-series-name=\\"test\\"><div class=\\"echLegendItem__color\\" aria-label=\\"series color\\" title=\\"series color\\"><div><svg xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"16\\" height=\\"16\\" class=\\"echIcon\\" color=\\"#1EA593\\" focusable=\\"false\\"><defs><circle id=\\"dot-a\\" cx=\\"8\\" cy=\\"8\\" r=\\"4\\"></circle></defs><g><use xlink:href=\\"#dot-a\\"></use></g></svg></div></div><div class=\\"echLegendItem__label echLegendItem__label--clickable\\" title=\\"test\\">test</div></li></ul></div></div><div class=\\"echContainer\\"><div class=\\"echChartPointerContainer\\" style=\\"cursor: default;\\"><div class=\\"echCrosshair\\"><div class=\\"echCrosshair__band\\" style=\\"top: -1px; left: -1px; width: 0px; height: 0px; background: rgb(245, 245, 245);\\"></div></div><canvas class=\\"echCanvasRenderer\\" width=\\"150\\" height=\\"200\\" style=\\"width: 150px; height: 200px;\\"></canvas><svg class=\\"echHighlighter\\"><defs><clipPath id=\\"echHighlighterClipPath__chart1\\"><rect x=\\"0\\" y=\\"0\\" width=\\"130\\" height=\\"180\\"></rect></clipPath></defs><g></g></svg></div></div></div>"`;
exports[`Chart should render the legend name test 1`] = `"<div class=\\"echChart\\" style=\\"width: 100px; height: 100px;\\"><div class=\\"echChartBackground\\" style=\\"background-color: transparent;\\"></div><div class=\\"echChartStatus\\" data-ech-render-complete=\\"true\\" data-ech-render-count=\\"1\\"></div><div class=\\"echChartResizer\\"></div><div class=\\"echLegend echLegend--right echLegend--debug\\"><div style=\\"width: 50px; max-width: 50px; margin-left: 0px; margin-right: 0px;\\" class=\\"echLegendListContainer\\"><ul style=\\"padding-top: 10px; padding-bottom: 10px;\\" class=\\"echLegendList\\"><li class=\\"echLegendItem echLegendItem--right\\" data-ech-series-name=\\"test\\"><div class=\\"echLegendItem__color\\" title=\\"series color\\"><svg xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"16\\" height=\\"16\\" class=\\"echIcon\\" color=\\"#1EA593\\" focusable=\\"false\\" aria-label=\\"series color: #1EA593\\"><defs><circle id=\\"dot-a\\" cx=\\"8\\" cy=\\"8\\" r=\\"4\\"></circle></defs><g><use xlink:href=\\"#dot-a\\"></use></g></svg></div><button type=\\"button\\" class=\\"echLegendItem__label echLegendItem__label--clickable\\" title=\\"test\\" aria-label=\\"test; Activate to hide series in graph\\">test</button></li></ul></div></div><div class=\\"echContainer\\"><div class=\\"echChartPointerContainer\\" style=\\"cursor: default;\\"><div class=\\"echCrosshair\\"><div class=\\"echCrosshair__band\\" style=\\"top: -1px; left: -1px; width: 0px; height: 0px; background: rgb(245, 245, 245);\\"></div></div><canvas class=\\"echCanvasRenderer\\" width=\\"150\\" height=\\"200\\" style=\\"width: 150px; height: 200px;\\"></canvas><svg class=\\"echHighlighter\\"><defs><clipPath id=\\"echHighlighterClipPath__chart1\\"><rect x=\\"0\\" y=\\"0\\" width=\\"130\\" height=\\"180\\"></rect></clipPath></defs><g></g></svg></div></div></div>"`;
6 changes: 3 additions & 3 deletions src/components/legend/__snapshots__/legend.test.tsx.snap

Large diffs are not rendered by default.

12 changes: 9 additions & 3 deletions src/components/legend/_legend.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@
&--top,
&--bottom {
.echLegendList {
display: grid;
display: flex;
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
grid-column-gap: $echLegendColumnGap;
grid-row-gap: $echLegendRowGap;
margin-top: $echLegendRowGap;
margin-bottom: $echLegendRowGap;

@include internetExplorerOnly {
display: flex;
flex-direction: row;
Expand All @@ -20,6 +19,7 @@
&--right {
.echLegendList {
flex-direction: column;
width: 100%;
}
}

Expand All @@ -42,6 +42,12 @@
@include euiYScrollWithShadows;
width: 100%;
overflow-y: auto;
overflow-x: hidden;

:focus {
@include euiFocusRing;
background-color: $euiFocusBackgroundColor;
border-radius: $euiBorderRadius / 2;
margin-right: $euiSizeXS;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The margin-right is making the horizontal items to move when they are focused.

Screen Recording 2020-11-17 at 12 53 PM

I think the reason you added this margin-right was for cases that we have the legend items added to the right. So we can be more specific and only add the margin-right to those cases:

Suggested change
:focus {
@include euiFocusRing;
background-color: $euiFocusBackgroundColor;
border-radius: $euiBorderRadius / 2;
margin-right: $euiSizeXS;
}
.echLegendItem--right :focus {
margin-right: $euiSizeXS;
}
:focus {
@include euiFocusRing;
background-color: $euiFocusBackgroundColor;
border-radius: $euiBorderRadius / 2;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙇‍♀️ thank you f751a99

Copy link
Collaborator

Choose a reason for hiding this comment

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

@miukimiu Is there a way we could add a transparent border to this so there is no jump in height and width when the border is added?

Screen Recording 2020-11-17 at 12 53 PM

}
}
6 changes: 5 additions & 1 deletion src/components/legend/_legend_item.scss
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ $legendItemVerticalPadding: $echLegendRowGap / 2;

&__color {
margin-right: $euiSizeXS;
margin-left: $euiSizeXS;

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that the color picker and the legend don't have the same height. And the circle is not vertically centered.

Screenshot 2020-11-17 at 12 18 12

To improve this we can add:

Suggested change
display: flex;
align-items: center;
// makes the color picker to have the same height as the legend
height: 18px;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the help! Commit f751a99 for changes

.euiPopover {
vertical-align: inherit; // prevents color dot from shifting
Expand All @@ -49,6 +50,9 @@ $legendItemVerticalPadding: $echLegendRowGap / 2;
@include euiFontSizeXS;
@include euiTextTruncate;
flex: 1 1 auto;
text-align: left;
overflow-x: scroll;
overflow: hidden;

&--clickable {
&:hover {
Expand All @@ -61,9 +65,9 @@ $legendItemVerticalPadding: $echLegendRowGap / 2;
&__extra {
@include euiFontSizeXS;
text-align: right;
flex: 0 0 auto;
margin-left: $euiSizeXS;
font-feature-settings: 'tnum';
flex: 0 0 auto;

&--hidden {
display: none;
Expand Down
34 changes: 18 additions & 16 deletions src/components/legend/color.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
* under the License.
*/

import classNames from 'classnames';
import React, { MouseEventHandler, forwardRef, memo } from 'react';

import { Icon } from '../icons/icon';
Expand All @@ -34,30 +33,33 @@ interface ColorProps {
* @internal
*/
export const Color = memo(
forwardRef<HTMLDivElement, ColorProps>(({ color, isSeriesHidden = false, hasColorPicker, onClick }, ref) => {
forwardRef<HTMLButtonElement, ColorProps>(({ color, isSeriesHidden = false, hasColorPicker, onClick }, ref) => {
if (isSeriesHidden) {
return (
<div className="echLegendItem__color" aria-label="series hidden" title="series hidden">
<div className="echLegendItem__color" title="series hidden">
{/* changing the default viewBox for the eyeClosed icon to keep the same dimensions */}
<Icon type="eyeClosed" viewBox="-3 -3 22 22" />
<Icon type="eyeClosed" viewBox="-3 -3 22 22" aria-label="series color is hidden" />
myasonik marked this conversation as resolved.
Show resolved Hide resolved
</div>
);
}

const colorClasses = classNames('echLegendItem__color', {
'echLegendItem__color--changable': hasColorPicker,
});
if (hasColorPicker) {
return (
<button
type="button"
onClick={onClick}
className="echLegendItem__color echLegendItem__color--changable"
title="change series color"
ref={ref}
>
<Icon type="dot" color={color} aria-label="change series color" />
myasonik marked this conversation as resolved.
Show resolved Hide resolved
</button>
);
}

return (
<div
onClick={hasColorPicker ? onClick : undefined}
className={colorClasses}
aria-label="series color"
title={hasColorPicker ? 'change series color' : 'series color'}
>
<div ref={ref}>
<Icon type="dot" color={color} />
</div>
<div className="echLegendItem__color" title="series color">
<Icon type="dot" color={color} aria-label={`series color: ${color}`} />
</div>
);
}),
Expand Down
15 changes: 12 additions & 3 deletions src/components/legend/label.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,28 @@ import React, { MouseEventHandler } from 'react';

interface LabelProps {
label: string;
isSeriesHidden?: boolean;
onClick?: MouseEventHandler;
}
/**
* Label component used to display text in legend item
* @internal
*/
export function Label({ label, onClick }: LabelProps) {
export function Label({ label, onClick, isSeriesHidden }: LabelProps) {
const labelClassNames = classNames('echLegendItem__label', {
'echLegendItem__label--clickable': Boolean(onClick),
});
return (
<div className={labelClassNames} title={label} onClick={onClick}>
<button
type="button"
className={labelClassNames}
title={label}
onClick={onClick}
aria-label={
isSeriesHidden ? `${label}; Activate to show series in graph` : `${label}; Activate to hide series in graph`
}
>
{label}
</div>
</button>
);
}
4 changes: 2 additions & 2 deletions src/components/legend/legend_item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export class LegendListItem extends Component<LegendItemProps, LegendItemState>
static displayName = 'LegendItem';
shouldClearPersistedColor = false;

colorRef = createRef<HTMLDivElement>();
colorRef = createRef<HTMLButtonElement>();
state: LegendItemState = {
isOpen: false,
actionActive: false,
Expand Down Expand Up @@ -235,7 +235,7 @@ export class LegendListItem extends Component<LegendItemProps, LegendItemState>
hasColorPicker={hasColorPicker}
onClick={this.handleColorClick(hasColorPicker)}
/>
<ItemLabel label={label} onClick={this.handleLabelClick(seriesIdentifier)} />
<ItemLabel label={label} onClick={this.handleLabelClick(seriesIdentifier)} isSeriesHidden={isSeriesHidden} />
{showExtra && extra != null && renderExtra(extra, isSeriesHidden)}
{Action && (
<div className="echLegendItem__action">
Expand Down
8 changes: 5 additions & 3 deletions stories/legend/11_legend_actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ const getAction = (hideActions: boolean, anchorPosition: PopoverAnchorPosition):
];

const Button = (
<div
<button
type="button"
style={{
display: 'flex',
justifyContent: 'center',
Expand All @@ -113,7 +114,7 @@ const getAction = (hideActions: boolean, anchorPosition: PopoverAnchorPosition):
onClick={() => setPopoverOpen(!popoverOpen)}
>
<EuiIcon size="s" type="pencil" />
</div>
</button>
);

return (
myasonik marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -125,6 +126,7 @@ const getAction = (hideActions: boolean, anchorPosition: PopoverAnchorPosition):
panelPaddingSize="none"
withTitle
anchorPosition={anchorPosition}
ownFocus
myasonik marked this conversation as resolved.
Show resolved Hide resolved
>
<EuiContextMenu initialPanelId={0} panels={getPanels(series as XYChartSeriesIdentifier)} />
</EuiPopover>
Expand All @@ -137,7 +139,7 @@ const renderColorPicker = (anchorPosition: PopoverAnchorPosition): LegendColorPi
onClose,
onChange,
}) => (
<EuiWrappingPopover isOpen button={anchor} closePopover={onClose} anchorPosition={anchorPosition}>
<EuiWrappingPopover isOpen button={anchor} closePopover={onClose} anchorPosition={anchorPosition} ownFocus>
<EuiColorPicker display="inline" color={color} onChange={onChange} />
<EuiSpacer size="m" />
<EuiButton fullWidth size="s" onClick={onClose}>
Expand Down
2 changes: 1 addition & 1 deletion stories/legend/9_color_picker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export const Example = () => {
onChangeAction(c);
};
return (
<EuiWrappingPopover isOpen button={anchor} closePopover={handleClose} anchorPosition="leftCenter">
<EuiWrappingPopover isOpen button={anchor} closePopover={handleClose} anchorPosition="leftCenter" ownFocus>
<EuiColorPicker display="inline" color={color} onChange={handleChange} />
<EuiSpacer size="m" />
<EuiFlexItem grow={false}>
Expand Down