[Vis: Default editor] EUIficate coordinate map (tile map) options tab #42517

sulemanof · 2019-08-02T12:38:08Z

Summary

A part of #38273.
EUIfication of the Options tab in the Coordinate map vis.

This includes:

created a common RangeOption component;
created a common TextInputOption component;
created withInjectedDependencies as HOC to pass dependencies from angular injector as serviceSettings to Options;
created WmsOptions and WmsInternalOptions components;
removed vislibBasicOptions directive;

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

…ns/tile_map

elasticmachine · 2019-08-02T12:39:33Z

Pinging @elastic/kibana-app

elasticmachine · 2019-08-02T13:43:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-02T16:07:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

src/legacy/core_plugins/tile_map/public/components/wms_interna_options.tsx

cchaos · 2019-08-02T17:15:01Z

src/legacy/core_plugins/tile_map/public/components/wms_interna_options.tsx

+      <EuiSpacer size="s" />
+
+      <TextInputOption
+        label={<FormattedMessage id="tileMap.wmsOptions.wmsUrlLabel" defaultMessage="WMS url*" />}


We need to try to make these "footnotes" accessible. I'm thinking that you'll want to do some form of:

<label>WMS url <sup aria-describedby="wmsInternalOptionsFootnote">*</sup></label> <p id="wmsInternalOptionsFootnote">*If this parameter is incorrect, maps will fail to load.</p>

Or maybe @myasonik has some ideas as well.

aria-label, aria-labelledby and aria-describedby generally don't work on non-interactive and/or non-widget role elements.

I tested adding role=tooltip to various elements but that's not well supported... (though that's probably the answer I wish I could give)

So I think the best recommendation I can give is to add this text to the help text of the inputs so it's read as part of the description of the input.

To clean up the reading, I'd also hide the asterisk and the final footnote from screen readers with aria-hidden=true.

Just a small note to add, also just noticed that we could pass in describedByIds which would similarly work to add more description text to the input but, after testing it with Daniil, we decided against it.

Using describedByIds puts the repeated footnote first and the unique help text second but a user is likely to navigate away from the input before it's done reading if it sounds like we're reading the same text over and over again.

Thanks @myasonik for your help!
The PR updated with suggested changes

src/legacy/core_plugins/tile_map/public/components/wms_interna_options.tsx

…ns/tile_map

elasticmachine · 2019-08-05T12:05:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

maryia-lapata

Code LGTM, but please resolve comments below.

src/legacy/core_plugins/tile_map/public/components/wms_internal_options.tsx

maryia-lapata · 2019-08-05T12:24:57Z

src/legacy/ui/public/vis/editors/default/vis_options_props.tsx

+export interface InjectedDependencies {
+  [key: string]: any;
+}
+export interface VisOptionsProps extends InjectedDependencies {


I wouldn't extend VisOptionsProps itself because it will influence other components (Pie, Tag cloud) allowing to pass almost any addition props. Instead of this let's create a separate interface for tile and region maps (e.g. VisMapOptionsProps) which will extend VisOptionsProps interface and have serviceSettings property additionally.

Nice catch! Done!

src/legacy/core_plugins/tile_map/public/components/wms_internal_options.tsx

cchaos

LGTM, with just one extra suggestion.

src/legacy/core_plugins/tile_map/public/components/wms_options.tsx

elasticmachine · 2019-08-05T14:04:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-05T15:07:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…elastic#42517) * EUIficate tile_map_vis_options * Add basic options * Get rid of vislib_basic_options * Move wms_options directive to region_map * Get rid if TileMapVisParams directive * Update region_map namespaces * Support of injecting of any dependency

…#42517) (#42671) * EUIficate tile_map_vis_options * Add basic options * Get rid of vislib_basic_options * Move wms_options directive to region_map * Get rid if TileMapVisParams directive * Update region_map namespaces * Support of injecting of any dependency

sulemanof added 11 commits July 30, 2019 12:32

EUIficate tile_map_vis_options

0ab813d

Merge remote-tracking branch 'upstream/master' into EUIfication/optio…

f3ab11e

…ns/tile_map

Add basic options

379a252

Merge remote-tracking branch 'upstream/master' into EUIfication/optio…

11c22ef

…ns/tile_map

Get rid of vislib_basic_options

eeb1ea5

Move wms_options directive to region_map

ebc08b1

Get rid if TileMapVisParams directive

ef27c00

Add wms options

e3e2439

Merge remote-tracking branch 'upstream/master' into EUIfication/optio…

2063d81

…ns/tile_map

Add wms_interna_options

c6a62d1

Merge remote-tracking branch 'upstream/master' into EUIfication/optio…

2aa78eb

…ns/tile_map

sulemanof requested a review from a team as a code owner August 2, 2019 12:38

sulemanof added Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0 labels Aug 2, 2019

sulemanof requested review from cchaos and maryia-lapata August 2, 2019 12:55

sulemanof added 2 commits August 2, 2019 16:59

Update region_map namespaces

5e2adbe

Add error handler

26c278d

timroes mentioned this pull request Aug 2, 2019

EUIficate visualization option panels #38273

Closed

10 tasks

Support of injecting of any dependency

c6e6336

sulemanof force-pushed the EUIfication/options/tile_map branch from 6c25782 to c6e6336 Compare August 2, 2019 15:06

cchaos suggested changes Aug 2, 2019

View reviewed changes

sulemanof added 2 commits August 5, 2019 10:52

Merge remote-tracking branch 'upstream/master' into EUIfication/optio…

d8ef67c

…ns/tile_map

Fix comments

fb28880

maryia-lapata approved these changes Aug 5, 2019

View reviewed changes

myasonik suggested changes Aug 5, 2019

View reviewed changes

src/legacy/core_plugins/tile_map/public/components/wms_internal_options.tsx Outdated Show resolved Hide resolved

Fix comments

585d293

sulemanof requested review from myasonik, cchaos, flash1293 and wylieconlon August 5, 2019 13:05

cchaos approved these changes Aug 5, 2019

View reviewed changes

src/legacy/core_plugins/tile_map/public/components/wms_options.tsx Show resolved Hide resolved

Fix comment

8b11d6f

myasonik approved these changes Aug 5, 2019

View reviewed changes

sulemanof merged commit 8eb99b4 into elastic:master Aug 6, 2019

sulemanof deleted the EUIfication/options/tile_map branch August 6, 2019 09:00

sulemanof mentioned this pull request Aug 6, 2019

[7.x] [Vis: Default editor] EUIficate coordinate map (tile map) options tab (#42517) #42671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vis: Default editor] EUIficate coordinate map (tile map) options tab #42517

[Vis: Default editor] EUIficate coordinate map (tile map) options tab #42517

sulemanof commented Aug 2, 2019 •

edited

Loading

elasticmachine commented Aug 2, 2019

elasticmachine commented Aug 2, 2019

elasticmachine commented Aug 2, 2019

cchaos Aug 2, 2019

myasonik Aug 5, 2019 •

edited

Loading

myasonik Aug 5, 2019

sulemanof Aug 5, 2019

elasticmachine commented Aug 5, 2019

maryia-lapata left a comment

maryia-lapata Aug 5, 2019

sulemanof Aug 5, 2019

cchaos left a comment

elasticmachine commented Aug 5, 2019

elasticmachine commented Aug 5, 2019

[Vis: Default editor] EUIficate coordinate map (tile map) options tab #42517

[Vis: Default editor] EUIficate coordinate map (tile map) options tab #42517

Conversation

sulemanof commented Aug 2, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 2, 2019

elasticmachine commented Aug 2, 2019

💔 Build Failed

elasticmachine commented Aug 2, 2019

💚 Build Succeeded

cchaos Aug 2, 2019

Choose a reason for hiding this comment

myasonik Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

myasonik Aug 5, 2019

Choose a reason for hiding this comment

sulemanof Aug 5, 2019

Choose a reason for hiding this comment

elasticmachine commented Aug 5, 2019

💚 Build Succeeded

maryia-lapata left a comment

Choose a reason for hiding this comment

maryia-lapata Aug 5, 2019

Choose a reason for hiding this comment

sulemanof Aug 5, 2019

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 5, 2019

💚 Build Succeeded

elasticmachine commented Aug 5, 2019

💚 Build Succeeded

sulemanof commented Aug 2, 2019 •

edited

Loading

myasonik Aug 5, 2019 •

edited

Loading