[Lens] Fix switching with layers #71982

wylieconlon · 2020-07-15T23:17:00Z

There were two issues with the chart switcher that caused it to remove all layers when using these steps:

Create any XY type of chart (i.e. Bar)
Add a second layer
Try switching to a different type of XY chart (i.e. Line)

There were two areas in the code that needed to be fixed:

The XY charts are no longer generating "mixed chart type" suggestions, which was previously possible for multiple layers. The chart switcher was filtering out these mixed chart types.
The chart switcher was showing error indicators for all multi-layer suggestions, which was too frequent. The new behavior only shows a warning when the layers are changing.

The new code also includes a functional test and unit test for the chart switching behavior with multiple layers.

Passes 42 times in flaky test runner: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/593/

Fixes #71372

…with-layers

wylieconlon · 2020-07-15T23:18:28Z

...ck/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

@@ -222,7 +222,7 @@ export function InnerWorkspacePanel({
                />
              </EuiLink>
            </small>
-          </p>
+          </div>


This <p> wrapping a <div> was causing errors in tests.

Before:

After:

😕 ? What does this fix? Using paragraphs is better DOM rendering for screen readers.

It fixes this warning:

console.error Warning: validateDOMNesting(...): <div> cannot appear as a descendant of <p>. in div (created by EuiIcon) in EuiIcon (created by Context.Consumer) in EuiI18n (created by EuiLink) in a (created by EuiLink) in EuiLink (created by InnerWorkspacePanel) in small (created by InnerWorkspacePanel) in p (created by InnerWorkspacePanel)

Those all stem from EuiIcon and it'll be hard to get rid of them until we change the element that EuiIcon renders. I'd rather keep the appropriate text elements for screen-readers when possible and I'll make an issue for this in EUI.

Ok this is weird, EuiIcon does not in fact ever render a <div> (I think it did way back when, but now it's just a simple <svg>). So this error message is actually completely false. Do we know who setup this DOM validator so I can follow up?

Ok, so I think I've tracked this down to the fact that in Kibana, because EuiIcon is dynamically imported, jest tests mock the component into rendering a <div>. So the React warning is actually remarking on the snapshots and not the actual dom. I'll see if the mocks are also coming from EUI, and if so, just get them to render a span.

But in the meantime, can you revert this p to div change? If you truly like the minimized space between the two lines, then wrap them all in a single <p> with a <br /> in between.

…with-layers

elasticmachine · 2020-07-16T16:04:23Z

Pinging @elastic/kibana-app (Team:KibanaApp)

cchaos

Awesome, thank you for fixing this. I tried out a couple scenarios and switching between the different x/y charts no matter how many layers seems to work fine.

cchaos · 2020-07-16T16:50:03Z

...ck/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

@@ -222,7 +222,7 @@ export function InnerWorkspacePanel({
                />
              </EuiLink>
            </small>
-          </p>
+          </div>


😕 ? What does this fix? Using paragraphs is better DOM rendering for screen readers.

flash1293

Tested in Firefox and works as expected, LGTM 👍

…with-layers

kibanamachine · 2020-07-17T18:02:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: bac7af1

Build metrics

page load bundle size

id	value	diff	baseline
lens	833.2KB	+314.0B	832.9KB

History

💚 Build #62325 succeeded a41ea50
💔 Build #62127 failed 3ed4b61

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* [Lens] Fix chart switching with multiple layers * Unskip Lens smokescreen test * Fix types * Revert <p> change

* master: [Observability] Remove app logos (elastic#72259) Fix float percentiles line chart (elastic#71902) update chromedriver to 84 (elastic#72228) [esArchiver] actually re-delete the .kibana index if we lose recreate race (elastic#72354) [Maps] convert SavedGisMap to TS (elastic#72286) [DOCS] Removes occurrences of X-Pack Security and Reporting (elastic#72302) use WORKSPACE env var for stack_functional_integration tests, fix navigate path (elastic#71908) [Monitoring] Fix issue with ES node detail status (elastic#72298) [SIEM] Updates consumer in export_rule archive (elastic#72324) [kbn/dev-utils] add RunWithCommands utility (elastic#72311) [Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes (elastic#72000) skip flaky suite (elastic#72339) [ML] Fix annotations pagination & change labels from letters to numbers (elastic#72204) [Lens] Fix switching with layers (elastic#71982) [Maps] 7.9 documenation updates (elastic#71893) docs: ✏️ add "Explore underlying data" user docs (elastic#70807) [Security Solution][Exceptions] - Remove initial add exception item button in builder (elastic#72215) Fix indentation level in code exploration doc (elastic#72274) register graph usage (elastic#72041) [Monitoring] Added a case for Alerting if security/ssl is disabled (elastic#71846)

wylieconlon added 3 commits July 15, 2020 19:07

[Lens] Fix chart switching with multiple layers

9819acb

Merge remote-tracking branch 'origin/master' into lens/fix-switching-…

a32f4c1

…with-layers

Unskip Lens smokescreen test

3ed4b61

wylieconlon added the Feature:Lens label Jul 15, 2020

wylieconlon commented Jul 15, 2020

View reviewed changes

wylieconlon added 2 commits July 16, 2020 11:36

Merge remote-tracking branch 'origin/master' into lens/fix-switching-…

a678514

…with-layers

Fix types

a41ea50

wylieconlon added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v7.9.0 v8.0.0 labels Jul 16, 2020

wylieconlon requested review from cchaos, flash1293 and mbondyra July 16, 2020 15:59

wylieconlon marked this pull request as ready for review July 16, 2020 16:04

wylieconlon requested a review from a team July 16, 2020 16:04

cchaos approved these changes Jul 16, 2020

View reviewed changes

flash1293 approved these changes Jul 17, 2020

View reviewed changes

wylieconlon added 2 commits July 17, 2020 12:07

Merge remote-tracking branch 'origin/master' into lens/fix-switching-…

9c55cd3

…with-layers

Revert <p> change

bac7af1

wylieconlon merged commit 35ff37a into elastic:master Jul 17, 2020

wylieconlon deleted the lens/fix-switching-with-layers branch July 17, 2020 18:02

This was referenced Jul 17, 2020

[7.x] [Lens] Fix switching with layers (#71982) #72326

Merged

[7.9] [Lens] Fix switching with layers (#71982) #72327

Merged

wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Jul 17, 2020

[Lens] Fix switching with layers (elastic#71982)

f3c6342

* [Lens] Fix chart switching with multiple layers * Unskip Lens smokescreen test * Fix types * Revert <p> change

wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Jul 17, 2020

[Lens] Fix switching with layers (elastic#71982)

48f10e1

* [Lens] Fix chart switching with multiple layers * Unskip Lens smokescreen test * Fix types * Revert <p> change

wylieconlon pushed a commit that referenced this pull request Jul 17, 2020

[Lens] Fix switching with layers (#71982) (#72327)

d7168a0

* [Lens] Fix chart switching with multiple layers * Unskip Lens smokescreen test * Fix types * Revert <p> change

wylieconlon pushed a commit that referenced this pull request Jul 17, 2020

[Lens] Fix switching with layers (#71982) (#72326)

b6235ec

* [Lens] Fix chart switching with multiple layers * Unskip Lens smokescreen test * Fix types * Revert <p> change

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Fix switching with layers #71982

[Lens] Fix switching with layers #71982

wylieconlon commented Jul 15, 2020 •

edited

Loading

wylieconlon Jul 15, 2020 •

edited

Loading

cchaos Jul 16, 2020

wylieconlon Jul 16, 2020

cchaos Jul 16, 2020

cchaos Jul 17, 2020

cchaos Jul 17, 2020

elasticmachine commented Jul 16, 2020

cchaos left a comment

cchaos Jul 16, 2020

flash1293 left a comment

kibanamachine commented Jul 17, 2020

[Lens] Fix switching with layers #71982

[Lens] Fix switching with layers #71982

Conversation

wylieconlon commented Jul 15, 2020 • edited Loading

wylieconlon Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

cchaos Jul 16, 2020

Choose a reason for hiding this comment

wylieconlon Jul 16, 2020

Choose a reason for hiding this comment

cchaos Jul 16, 2020

Choose a reason for hiding this comment

cchaos Jul 17, 2020

Choose a reason for hiding this comment

cchaos Jul 17, 2020

Choose a reason for hiding this comment

elasticmachine commented Jul 16, 2020

cchaos left a comment

Choose a reason for hiding this comment

cchaos Jul 16, 2020

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 17, 2020

💚 Build Succeeded

Build metrics

page load bundle size

History

wylieconlon commented Jul 15, 2020 •

edited

Loading

wylieconlon Jul 15, 2020 •

edited

Loading