[Canvas] Add simple variables to workpads by poffdeluxe · Pull Request #66139 · elastic/kibana

poffdeluxe · 2020-05-11T22:51:52Z

Summary

Adds variables to Canvas workpads by consuming var function added to the expressions plugin.

At the moment, three variable types are supported: boolean, number, and string.

STILL TODO:

~~a11y work~~
~~UI cleanup~~
~~Double check workpad migration~~

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
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-05-12T14:52:36Z

Pinging @elastic/kibana-canvas (Team:Canvas)

x-pack/plugins/canvas/public/functions/filters.ts

x-pack/plugins/canvas/public/components/datasource/datasource_preview/index.js

andreadelrio

Some comments about using Eui variables. Avoid targeting Eui classes directly whenever possible (not sure you can do that for the table's header and footer but at least try to pass a custom class to the table so you don't target .euiTable).

Also left you a suggestion for the variables' accordion.

x-pack/plugins/canvas/public/components/arg_form/arg_form.scss

x-pack/plugins/canvas/public/components/var_config/edit_var.scss

x-pack/plugins/canvas/public/components/var_config/var_config.scss

andreadelrio · 2020-07-02T22:58:37Z

x-pack/plugins/canvas/public/components/var_config/var_config.scss

+  position: relative;
+  left: 0;
+
+  transition-property: left;


This transition seems a bit overkill for the space. I'd also keep the accordion's header visible at all times. I don't think you need the back arrow. The Cancel button should be enough. This will result in something like this

Thanks for the comment, @andreadelrio! For my part, the panel animation and back button inclusion were both intentional design decisions. Our thinking for recommending them was to give the user the impression that they are traversing one level deeper into the variables panel experience (almost like a modal-within-a-modal experience). The hope was that the animation and back button both help convey this feeling to the user. The back button also affords the user one additional way in which they can remove themselves from the experience, which I don't consider a bad thing.

Regarding keeping the accordion header visible at all times, our thinking was that it is unnecessary to continue to allow the user the ability to collapse a panel in which they have just made an action that tells us they wish to interact with the panel further (via changes to a variable). The hope was that replacing the accordion header with the back arrow and the current action's title maintains a better, simple panel hierarchy, while also further promoting the ideas above. Thoughts?

@MichaelMarcialis Thanks for explaining that. When I thought about keeping the accordion header visible I didn't think "let's continue to allow them to collapse that panel" but more about "let's give them context and remind them where they are" i.e. Variables > Edit Variable. So in that sense, I think keeping the title could help communicate the existing hierarchy more clearly.

I agree that the back arrow can also help communicate that hierarchy and if you keep it, I would remove the accordion header. I think my main concern is that adding the left to right animation makes the interface feel a bit bloated. Maybe we could use another animation/transition.

Cool. Just to dig into that a bit more, is it specifically the left-to-right animation that you dislike? Or is it more about timing/easing/etc? I can totally see the feeling of bloat if you feel the transition is a little too sluggish.

Cool. Just to dig into that a bit more, is it specifically the left-to-right animation that you dislike? Or is it more about timing/easing/etc? I can totally see the feeling of bloat if you feel the transition is a little too sluggish.

It's the timing/easing. It doesn't feel super smooth.

+1 to the animation being a bit rough. In testing this, it seems to freeze mid-way through. If we're going to keep it, then I agree it needs some refinement.

x-pack/plugins/canvas/public/components/var_config/var_panel.scss

ppisljar

app arch changes lgtm

src/plugins/expressions/common/execution/execution.ts

poffdeluxe · 2020-07-06T17:46:21Z

@elasticmachine merge upstream

elasticmachine · 2020-07-06T17:46:23Z

merge conflict between base and head

crob611 · 2020-07-08T15:36:52Z

A few visual questions/issues

Do True/False need translations?

If the list is sorted by variable type, then the type header is truncated

Does the copy snippet need it's own icon AND to be in the triple dot menu?

MichaelMarcialis

Thanks for making those suggested changes, @poffdeluxe. Looks great! Approving.

If I get some time tomorrow, I'll try to join you in fiddling around with our animation options to see if we can smooth the sliding animation out a bit.

poffdeluxe · 2020-07-13T15:06:47Z

@elasticmachine merge upstream

ryankeairns

As evidenced by the gif below, there is still a rather ugly state of the UI that occurs right after creation and deletion of a variable (note: this is in Firefox)

I suppose there could be differing opinions on whether or not this is a showstopper (since it all works fine), but for me it falls below our design threshold since it appears momentarily broken.

Is there anything else that can be done for that momentary glitch right after create/delete? It looks like the form height shrinks and exposes part of the incoming panel before the animation begins. Perhaps showing a loading div or something?

…to canvas-workpad-vars

ryankeairns · 2020-07-13T16:49:32Z

We opted to remove the animation for now given the glitches and inconsistencies which appeared for me in both Firefox and Chrome. The design itself makes sense in that it provides some additional context as the form appears, but we need more time to experiment than what remains in this release.

The feature itself works well, so lets revisit the animation enhancement post FF.

clintandrewhall · 2020-07-13T17:20:14Z

nice work, @poffdeluxe and @ryankeairns...!

@andreadelrio Do you have any further changes that need to be made, or can this PR be approved on your part?

poffdeluxe · 2020-07-13T18:54:37Z

@elasticmachine merge upstream

kibanamachine · 2020-07-13T20:33:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0a78366

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
canvas	826	+27	799

History

💔 Build #60961 failed 0face68
💚 Build #60910 succeeded 86fb447
💚 Build #60524 succeeded 0fa9bed
💔 Build #60455 failed 1a8c202
💚 Build #59928 succeeded c7291be

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

* Add simple variables to Canvas workpads * Fix type for workpad variable action and clarify comment * Fix types in fixtures and templates * Fixing type check errors on actions * Addressing pr feedback and refactoring canvas sidebar accordions * Render true/false instead of Yes/no on variables * add warning callout when editing a variable * Address review feedback * More feedback * updating storyshot with new edit mode callout * Some animation tweaks for the panel * one more panel tweak * Removing the slide transition for now Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label May 11, 2020

thomasneirynck added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// label May 12, 2020

poffdeluxe force-pushed the canvas-workpad-vars branch from 0c40496 to 1d66696 Compare May 28, 2020 21:00

poffdeluxe force-pushed the canvas-workpad-vars branch from 1d66696 to c7e8c3f Compare June 9, 2020 16:56

poffdeluxe force-pushed the canvas-workpad-vars branch from baaf27d to 19de317 Compare June 29, 2020 20:21

poffdeluxe commented Jul 1, 2020

View reviewed changes

x-pack/plugins/canvas/public/functions/filters.ts Outdated Show resolved Hide resolved

poffdeluxe commented Jul 1, 2020

View reviewed changes

x-pack/plugins/canvas/public/components/datasource/datasource_preview/index.js Outdated Show resolved Hide resolved

poffdeluxe changed the title ~~Add simple variables to Canvas workpads~~ [Canvas] Add simple variables to Canvas workpads Jul 2, 2020

poffdeluxe force-pushed the canvas-workpad-vars branch from 2382b7c to db09ca4 Compare July 2, 2020 21:00

Add simple variables to Canvas workpads

62e2e9d

poffdeluxe force-pushed the canvas-workpad-vars branch from db09ca4 to 62e2e9d Compare July 2, 2020 21:04

poffdeluxe added enhancement New value added to drive a business result impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:enhancement review v7.9.0 v8.0.0 labels Jul 2, 2020

Fix type for workpad variable action and clarify comment

b08895f

poffdeluxe marked this pull request as ready for review July 2, 2020 21:10

poffdeluxe requested review from a team as code owners July 2, 2020 21:10

andreadelrio requested changes Jul 2, 2020

View reviewed changes

ppisljar approved these changes Jul 6, 2020

View reviewed changes

ppisljar reviewed Jul 6, 2020

View reviewed changes

src/plugins/expressions/common/execution/execution.ts Outdated Show resolved Hide resolved

Fix types in fixtures and templates

8de85ff

Address review feedback

331d9c3

MichaelMarcialis approved these changes Jul 8, 2020

View reviewed changes

poffdeluxe added 6 commits July 8, 2020 14:07

More feedback

5afa705

Merge branch 'master' into canvas-workpad-vars

feb435e

updating storyshot with new edit mode callout

c7291be

Some animation tweaks for the panel

1a8c202

Merge branch 'master' into canvas-workpad-vars

985de7e

one more panel tweak

0fa9bed

Merge branch 'master' into canvas-workpad-vars

86fb447

ryankeairns self-requested a review July 13, 2020 15:10

ryankeairns reviewed Jul 13, 2020

View reviewed changes

poffdeluxe added 2 commits July 13, 2020 11:25

Removing the slide transition for now

fb10480

Merge branch 'canvas-workpad-vars' of github.com:poffdeluxe/kibana in…

0face68

…to canvas-workpad-vars

ryankeairns self-requested a review July 13, 2020 16:45

ryankeairns approved these changes Jul 13, 2020

View reviewed changes

Merge branch 'master' into canvas-workpad-vars

0a78366

andreadelrio approved these changes Jul 13, 2020

View reviewed changes

poffdeluxe merged commit 4d6ad89 into elastic:master Jul 13, 2020

poffdeluxe deleted the canvas-workpad-vars branch July 13, 2020 20:45

poffdeluxe mentioned this pull request Jul 13, 2020

[7.x] [Canvas] Add simple variables to workpads (#66139) #71538

Merged

This was referenced Jul 15, 2020

[Canvas] Re-enable panel animation for Workpad Variables #71877

Closed

[Canvas] Add workpad-level variables to Canvas workpads #67058

Closed

cqliu1 mentioned this pull request Aug 4, 2020

[Canvas][Docs] Adds var and var_set to expression function reference #74291

Merged

2 tasks

Conversation

poffdeluxe commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented May 12, 2020

Uh oh!

Uh oh!

Uh oh!

andreadelrio left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreadelrio Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

MichaelMarcialis Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

andreadelrio Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

MichaelMarcialis Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

andreadelrio Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

ryankeairns Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

poffdeluxe commented Jul 6, 2020

Uh oh!

elasticmachine commented Jul 6, 2020

Uh oh!

crob611 commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelMarcialis left a comment

Choose a reason for hiding this comment

Uh oh!

poffdeluxe commented Jul 13, 2020

Uh oh!

ryankeairns left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryankeairns commented Jul 13, 2020

Uh oh!

clintandrewhall commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poffdeluxe commented Jul 13, 2020

Uh oh!

kibanamachine commented Jul 13, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

poffdeluxe commented May 11, 2020 •

edited

Loading

andreadelrio left a comment •

edited

Loading

crob611 commented Jul 8, 2020 •

edited

Loading

ryankeairns left a comment •

edited

Loading

clintandrewhall commented Jul 13, 2020 •

edited

Loading