[Lens] (Accessibility) Added button to execute drag and drop to workspace #85960

mbondyra · 2020-12-15T14:26:57Z

Summary

Fixes #83730
Fixes #83600

Description of functionality here.

Checklist

Delete any items that are not applicable to this PR.

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

MichaelMarcialis · 2020-12-17T21:31:31Z

Hey, @mbondyra! Per your request, I've made a quick design PR for your branch. Have a look here whenever you get a moment:

mbondyra#4

…_workspace_design [Lens] "Add field to workspace" button design tweaks

mbondyra · 2020-12-18T14:04:56Z

@elasticmachine merge upstream

LeeDr · 2021-01-07T22:53:54Z

Since we're past 7.11.0 FF, and this isn't a bug fix, should probably bump to 7.12.0.

mbondyra · 2021-01-11T08:47:46Z

@elasticmachine merge upstream

…y/dnd_to_workspace

flash1293 · 2021-01-11T15:59:38Z

@elasticmachine merge upstream

…dyra/kibana into lens/accessibility/dnd_to_workspace

flash1293 · 2021-01-12T09:11:08Z

@mbondyra I fixed types and tests (and restructured the code a little), could you check whether everything is still working as intended from your side?

wylieconlon

I tested this behavior and found that it worked consistently with what we have on master, so that part LGTM. The organization of this code makes sense too by moving this to the frame level. The only comment I have is that you've taken tests out without adding equivalent tests, and I'd like to make sure we keep those.

wylieconlon · 2021-01-12T21:03:08Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_helpers.ts

+    field,
+    mainPalette,
+  });
+  return suggestions.find((s) => s.visualizationId === activeVisualizationId) || suggestions[0];


I tried to break the suggestion logic here, and it does seem to be working consistently with what we have on master, but might benefit from some extra tests. It seems like we are intentionally switching the subVisualizationId on some chart types, like between donut -> pie when you add a second agg. Is this tested?

wylieconlon · 2021-01-12T21:06:54Z

x-pack/plugins/lens/public/indexpattern_datasource/field_item.tsx

+          }}
+          title={buttonTitle}
+        />
+      </EuiFlexItem>


I think I would have appreciated an EuiTooltip here instead of a title property- it seems like this button is potentially confusing for mouse users.

mbondyra · 2021-01-13T08:58:26Z

The behavior looks good to me, couldn't find any edgecases. I agree with Wylie's comment about tests tooltip. And just like we discussed, about adding disabled button to not have this state:

…y/dnd_to_workspace

flash1293 · 2021-01-15T09:29:08Z

Thanks for the review @wylieconlon - I think the tests are all covered now, although structured differently:
I removed the following:

should refuse to drop if there only suggestions from other visualizations if there are data tables - this is covered by should not consider suggestion from other visualization if there is data
should allow to drop if there are suggestions from active visualization even if there are data tables - covered by should return top suggestion for field
should immediately transition to the first suggestion if there are multiple - also tested by should immediately transition to the first suggestion if there are multiple (because mockVisualization1 returns two suggestions)

About

I tried to break the suggestion logic here, and it does seem to be working consistently with what we have on master, but might benefit from some extra tests. It seems like we are intentionally switching the subVisualizationId on some chart types, like between donut -> pie when you add a second agg. Is this tested?

We are picking the top suggestion of the current data source as tested in the new suggestion_helpers tests - at that point we don't check the sub visualization id at all because that's an implementation detail of the visualization and transparent for the frame logic under test (the visualization itself is mocked out). Am I missing something?

I also added tooltips and keep the button in a disabled state if it can't be used right now:

elasticmachine · 2021-01-15T16:58:12Z

Pinging @elastic/kibana-accessibility (Project:Accessibility)

elasticmachine · 2021-01-15T16:58:14Z

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

flash1293 · 2021-01-18T08:03:10Z

@elasticmachine merge upstream

flash1293

LGTM as I wrote parts of it, waiting for design review

…y/dnd_to_workspace

flash1293 · 2021-01-19T12:53:35Z

As discussed with @mbondyra the popover is closed now on adding it to the workspace to make it possible to see all of the resulting chart and to circumvent a focus bug in firefox

mbondyra · 2021-01-19T15:48:56Z

Checked again - the bug is fixed, approved!

myasonik

Two nits but works well! 🚀

x-pack/plugins/lens/public/indexpattern_datasource/field_item.tsx

myasonik · 2021-01-19T20:23:50Z

x-pack/plugins/lens/public/indexpattern_datasource/field_item.tsx

+          onClick={() => {
+            dropOntoWorkspace(field);
+          }}
+          title={buttonTitle}


Nit: With EuiToolTip already in place here, I think we can get rid of this

MichaelMarcialis

LGTM as I wrote parts of it, waiting for design review

@flash1293: A design PR from me with my suggested changes was merged into this branch earlier. This still looks good to me, with the exception of one new bug that I noticed today.

It seems that the "Add field to workspace" buttons can occasionally (and perhaps randomly) be immediately auto-focused when the field popover is opened. The button focus then causes the button's tooltip to show. It appears to only happen when opening the popover via mouse click on the field. It does not appear to happen when using the keyboard to open the popover. Is this a known issue?

wylieconlon

Changes LGTM, although I agree with @myasonik on the small tweaks

…y/dnd_to_workspace

flash1293 · 2021-01-20T13:10:28Z

Thanks for the reviews. I addressed Michails remarks and fixed Michaels bug

kibanamachine · 2021-01-20T15:28:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 916abf3

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.1MB	1.1MB	+5.4KB

History

💚 Build #99608 succeeded b52bf78
💔 Build #99598 failed e433808
💚 Build #99548 succeeded ec6817e
💚 Build #99348 succeeded ec6817e
💚 Build #99062 succeeded 6e9acc8

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

MichaelMarcialis

Thanks for fixing that bug, @flash1293. LGTM!

…pace (elastic#85960)

…pace (#85960) (#88853)

mbondyra added Project:Accessibility Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.11.0 labels Dec 15, 2020

[Lens] (Accessibility) button to drag and drop to workspace

821b695

mbondyra force-pushed the lens/accessibility/dnd_to_workspace branch from 2116e9a to 821b695 Compare December 15, 2020 14:28

mbondyra mentioned this pull request Dec 15, 2020

[Lens] (Accessibility) No way to recreate "drag to workspace" with keyboard #83730

Closed

update design for add to workspace button addition

ab4040b

Merge pull request #4 from MichaelMarcialis/lens/accessibility/dnd_to…

7e9a78e

…_workspace_design [Lens] "Add field to workspace" button design tweaks

Merge branch 'master' into lens/accessibility/dnd_to_workspace

10fa1e9

mbondyra added v7.12.0 and removed v7.11.0 labels Jan 11, 2021

kibanamachine and others added 2 commits January 11, 2021 03:47

Merge branch 'master' into lens/accessibility/dnd_to_workspace

c2b5228

Merge remote-tracking branch 'upstream/master' into lens/accessibilit…

c9b3692

…y/dnd_to_workspace

kibanamachine and others added 3 commits January 11, 2021 10:59

Merge branch 'master' into lens/accessibility/dnd_to_workspace

7a997c1

clean up pr

a569879

Merge branch 'lens/accessibility/dnd_to_workspace' of github.com:mbon…

0f04806

…dyra/kibana into lens/accessibility/dnd_to_workspace

wylieconlon reviewed Jan 12, 2021

View reviewed changes

flash1293 added 2 commits January 15, 2021 09:47

Merge remote-tracking branch 'upstream/master' into lens/accessibilit…

cd505c1

…y/dnd_to_workspace

fix tests and add tooltip

6e9acc8

mbondyra marked this pull request as ready for review January 15, 2021 16:58

mbondyra requested a review from a team January 15, 2021 16:58

mbondyra requested a review from a team as a code owner January 15, 2021 16:58

Merge branch 'master' into lens/accessibility/dnd_to_workspace

ec6817e

flash1293 approved these changes Jan 18, 2021

View reviewed changes

flash1293 added 2 commits January 19, 2021 12:21

Merge remote-tracking branch 'upstream/master' into lens/accessibilit…

8dc90ef

…y/dnd_to_workspace

close popover on field add

e433808

fix code style

b52bf78

myasonik approved these changes Jan 19, 2021

View reviewed changes

MichaelMarcialis reviewed Jan 19, 2021

View reviewed changes

wylieconlon approved these changes Jan 20, 2021

View reviewed changes

flash1293 added 2 commits January 20, 2021 13:42

Merge remote-tracking branch 'upstream/master' into lens/accessibilit…

73f5c78

…y/dnd_to_workspace

review comments

916abf3

MichaelMarcialis approved these changes Jan 20, 2021

View reviewed changes

flash1293 merged commit 0cd8c90 into elastic:master Jan 20, 2021

flash1293 pushed a commit to flash1293/kibana that referenced this pull request Jan 20, 2021

[Lens] (Accessibility) Added button to execute drag and drop to works…

184bfc7

…pace (elastic#85960)

flash1293 mentioned this pull request Jan 20, 2021

[7.x] [Lens] (Accessibility) Added button to execute drag and drop to workspace (#85960) #88853

Merged

flash1293 added a commit that referenced this pull request Jan 20, 2021

[Lens] (Accessibility) Added button to execute drag and drop to works…

d476b42

…pace (#85960) (#88853)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] (Accessibility) Added button to execute drag and drop to workspace #85960

[Lens] (Accessibility) Added button to execute drag and drop to workspace #85960

mbondyra commented Dec 15, 2020 •

edited

Loading

MichaelMarcialis commented Dec 17, 2020 •

edited

Loading

mbondyra commented Dec 18, 2020

LeeDr commented Jan 7, 2021

mbondyra commented Jan 11, 2021

flash1293 commented Jan 11, 2021

flash1293 commented Jan 12, 2021

wylieconlon left a comment

wylieconlon Jan 12, 2021

wylieconlon Jan 12, 2021

mbondyra commented Jan 13, 2021

flash1293 commented Jan 15, 2021

elasticmachine commented Jan 15, 2021

elasticmachine commented Jan 15, 2021

flash1293 commented Jan 18, 2021

flash1293 left a comment •

edited

Loading

flash1293 commented Jan 19, 2021

mbondyra commented Jan 19, 2021

myasonik left a comment

myasonik Jan 19, 2021

MichaelMarcialis left a comment

wylieconlon left a comment •

edited

Loading

flash1293 commented Jan 20, 2021

kibanamachine commented Jan 20, 2021

MichaelMarcialis left a comment

[Lens] (Accessibility) Added button to execute drag and drop to workspace #85960

[Lens] (Accessibility) Added button to execute drag and drop to workspace #85960

Conversation

mbondyra commented Dec 15, 2020 • edited Loading

Summary

Checklist

For maintainers

MichaelMarcialis commented Dec 17, 2020 • edited Loading

mbondyra commented Dec 18, 2020

LeeDr commented Jan 7, 2021

mbondyra commented Jan 11, 2021

flash1293 commented Jan 11, 2021

flash1293 commented Jan 12, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Jan 12, 2021

Choose a reason for hiding this comment

wylieconlon Jan 12, 2021

Choose a reason for hiding this comment

mbondyra commented Jan 13, 2021

flash1293 commented Jan 15, 2021

elasticmachine commented Jan 15, 2021

elasticmachine commented Jan 15, 2021

flash1293 commented Jan 18, 2021

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 commented Jan 19, 2021

mbondyra commented Jan 19, 2021

myasonik left a comment

Choose a reason for hiding this comment

myasonik Jan 19, 2021

Choose a reason for hiding this comment

MichaelMarcialis left a comment

Choose a reason for hiding this comment

wylieconlon left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 commented Jan 20, 2021

kibanamachine commented Jan 20, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

MichaelMarcialis left a comment

Choose a reason for hiding this comment

mbondyra commented Dec 15, 2020 •

edited

Loading

MichaelMarcialis commented Dec 17, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading

wylieconlon left a comment •

edited

Loading