[Visualize] First version of by-value visualize editor #72256

majagrubic · 2020-07-17T11:18:41Z

What is this?

This PR implements editing visualization by value inside dashboard and returning the edited visualization back to dashboard.

The idea is as follows:

We take the embeddable input from the dashboard and pass it (along with the original embeddable id) to the visualize editor via state-transfer service
Visualize editor reads the input from the state transfer service and renders the visualize embeddable (+ editor controls) from the input
Once the changes are applied, visualize editor passes the updated input together with the original embeddable id back to the dashboard
Dashboard updates the existing embeddable from the input it received

Note: Sharing of the visualization by-value doesn't work yet.

Implementation details

To support this flow, I introduced a new edit_by_value URL and a new visualize_byValue_editor. When editing visualization by value, there is no need to sync state from the URL, so the flow becomes a bit simpler. Common functionality was extracted to common components/functions, but some code duplication is probably still present.

What didn't change?

There should be no changes to the existing visualize editor. If something is wrong with editing visualizations from saved objects, it's likely a bug not a feature :)

How to test this?

The main focus should be on testing the existing visualize editor and that nothing is broken.
To test the new functionality, you'll need to set the showNewVisualizeFlow config value to true:

kibana/src/plugins/visualize/config.ts

Line 23 in f5061c7

showNewVisualizeFlow: schema.boolean({ defaultValue: false }),

This requires a server restart!
Once the flag has been enabled:
a) create a dashboard
b) create a new visualization (any visualization except Lens) and add it to dashboard
c) from the panel action menu select "Edit visualization" -> this should trigger the new "by-value" editor

Where are functional/unit tests?

Incoming!

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

Fixing broken behavior and applying relevant changes Adding changes to dashboard Removing unnecessary empty line Removing unnecessary deepClone Fixing some stuff in dashboard container Extracting logic into common components Fixing eslint Fix breadcrumbs Fixing error in search interceptor Reintroduce mistakenly removed empty lines Renaming function

elasticmachine · 2020-07-20T20:20:40Z

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

majagrubic · 2020-07-20T20:21:10Z

@elasticmachine merge upstream

ppisljar

app arch changes code LGTM

sulemanof

Tested basic behavior of Visualize app, I can confirm it works well after these changes. I didn't notice any regressions, except next one:

the Save as button works incorrect when edit a visualization through a dashboard panel:

actual behavior - copies a vis without any requested info;
expected behavior - modal appears:

About edit_by_value feature, I noticed couple of confusing things:

using browser's back/forward buttons, I trap into an empty screen since the visualization can't be loaded without data. The same thing occur if I reload a page.

My suggestion is to play around with the url tracker (e.x. erase the visited visualize page or replace with the actual dashbord);
lack of visualization title when created through the dashboard in edit by value mode;
can't unlink from the saved search ( details here );

sulemanof · 2020-07-22T14:46:28Z

src/plugins/visualize/public/application/app.tsx

@@ -48,6 +53,9 @@ export const VisualizeApp = () => {

  return (
    <Switch>
+      <Route exact path={`${VisualizeConstants.EDIT_BY_VALUE_PATH}`}>
+        <VisualizeByValueEditor />


What if you try instead to use the same <VisualizeEditor /> at this route, but use another effects inside when you trap into /edit_by_value path? It seems this will help to get rid of VisualizeEditorCommon ccomponent and code duplications between VisualizeByValueEditor <---> VisualizeEditor

I am not sure how? Hooks need to be run in order and they don't like branching statements. I thought having separate components was actually cleaner.

src/plugins/visualize/public/application/utils/breadcrumbs.ts

sulemanof · 2020-07-22T15:13:59Z

src/plugins/visualize/public/application/components/visualize_byvalue_editor.tsx

+    eventEmitter,
+    byValueVisInstance
+  );
+  const { isEmbeddableRendered, currentAppState } = useEditorUpdates(


The useEditorUpdates subscribes on the appState updates (which can be changed through the url).
Seems you don't need that subscription at all?

src/plugins/visualize/public/application/utils/get_top_nav_config.tsx

sulemanof · 2020-07-22T15:26:11Z

src/plugins/visualize/public/application/utils/use/use_editor_updates.ts

          delete savedVis.savedSearchId;
          delete vis.data.savedSearchId;
+        } else {
+          // TODO: something to do for when it's not a saved vis?


Should it be resolved before merging?
Unlinking isn't working right now in edit_by_value mode.

I'd really like to keep this PR as small as possible and build on it incrementally. I believe for the first pass it can work without saved search linking/unlinking.

mistic · 2020-08-10T12:51:57Z

@elasticmachine merge upstream

majagrubic · 2020-08-13T13:56:40Z

Hey @sulemanof, this should be ready for another review. I addressed 3 main concerns from the last review:

The problem with "Save As" appeared only if you upgrade from "old flow" to the "new flow" -> this shouldn't affect the existing visualize editor (but is still fixed, nonetheless)
Unlinking saved search works now
As for the getting stuck in the "edit_by_value" empty mode: @ThomThomson and I had a chat about this; the proper solution would include storing the vis in local storage, which we decided against when building state transfer service -> it would basically involve ditching state transfer service and coming up with a better solution; the hack we came up with is just: if there's no input when initializing the editor, we just return back to dashboard; we even change the breadcrumbs when opening the visualize editor from the dashboard, so that it's prefixed by "Dashboard" -> that way we make it more explicit that the user is still inside the dashboard space

As for two separate editors, I'd like to keep it this way for now. I think the separation of concerns is pretty good and it's following the implementation of the original visualize editor.

Also, another side note: This is not getting released soon, so we can make incremental changes in the meantime.

ThomThomson

Tested in chrome, and spent a little while going over the code.

This is looking quite stable overall! Splitting up the editor into three components actually resulted in far less code duplication than I thought it would, which is good to see.

One issue that I noticed is that editing a by reference visualization, then hitting save and return will duplicate the embeddable. I think what's happening here is that the visualize editor is accidentally putting the by value version of the embeddable into the state transfer service when it should just save the vis by reference and redirect back to dashboard.

Other than that, I have some additional small comments/suggestions that could be handled in this PR, or in a follow-up.

Unified Config Setting: It would be awesome for us to tie all of the embeddable by value changes behind a single config setting. I added one to the dashboard start contract recently and will update my lens PR to reference that one.

kibana/src/plugins/dashboard/config.ts

Line 23 in 5b64a4c

allowByValueEmbeddables: schema.boolean({ defaultValue: false }),

Attribute Service / Interface: Most likely a follow up PR, but implementing the ReferenceOrValueEmbeddable interface should be a priority so visualize embeddables work correctly with the unlink & add to library actions right out of the gate. Additionally, it'll be good to keep in mind that using the attribute service can make this implementation a lot easier.

Cancel Button: Should be a super easy addition in another PR. #70650

Save and return / Save as I think it's important to allow the user to opt out of the by value paradigm through the editor if they want to. To that end, I switched the menu bar UI in the Lens PR to include both the 'save and return' and 'save as' buttons, any time the user has arrived from a dashboard. This results in greater consistency as well, and it would be ideal if the menus worked the same in both visualize and lens.

Dashboard breadcrumb: Great implementation on this one, I'm definitely going to do the same in the lens PR. Wondering how hard it would be to make the dashboard breadcrumb into a link, which would do the same thing as the above mentioned cancel button.

Let me know when I can re-review, very excited to get this in!

majagrubic · 2020-08-17T12:03:05Z

Thanks for review @ThomThomson ! great catch about the duplicate embeddable being created, I believe this has been fixed now. To address some other concerns:
Unified Config Setting -> I'm not sure if a plugin can read config settings from another plugin. From my understanding, each plugin will have to have its own
Attribute Service / Interface: -> definitely the next item on my todo list
Save as -> I think this is related to the previous item; it will basically be "add to library functionality"
Dashboard breadcrumb -> I think the problem is the same as the one above; it's not possible to access dashboard link from visualize plugin

kibanamachine · 2020-08-17T12:41:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 228d007

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
visualize	311	+3	308

async chunks size

id	value	diff	baseline
visualize	693.8KB	+16.4KB	677.4KB

page load bundle size

id	value	diff	baseline
dashboard	687.9KB	+169.0B	687.7KB
embeddable	428.7KB	+179.0B	428.6KB
navigation	164.7KB	+26.0B	164.7KB
visualizations	409.0KB	+15.0B	409.0KB
visualize	31.0KB	+36.0B	30.9KB
total		+425.0B

History

💚 Build #68346 succeeded 1259c98
💚 Build #67927 succeeded 1741aae
💚 Build #67874 succeeded 29c6a31
💔 Build #67742 failed 875be62
💚 Build #67532 succeeded d4b69dc

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

ThomThomson

The duplication issue is fixed, and everything is working well - super excited to get this in!
LGTM.

A few notes for the followup PR questions:

Unified Config Setting: I actually exposed this setting in the dashboard start contract specifically so that other plugins can get access to it. The Lens PR doesn't have its own config setting anymore, and is using the dashboard one instead.

Dashboard breadcrumb: We actually already have an easy way to get back to the dashboard due to the 'originatingApp' parameter. It could call something like this:
coreStart.application.navigateToApp(embeddableEditorIncomingState?.originatingApp);

…ity for by value embeddables

* First version of new by-value editor Fixing broken behavior and applying relevant changes Adding changes to dashboard Removing unnecessary empty line Removing unnecessary deepClone Fixing some stuff in dashboard container Extracting logic into common components Fixing eslint Fix breadcrumbs Fixing error in search interceptor Reintroduce mistakenly removed empty lines Renaming function * Adding missing null check * Making typescript play nicely * Fixing failing tests * Applying PR comments * Fixing eslint errors * Fix save as behavior * Fixing HTMLElement type * Passing in setOriginatingApp parameter * Redirect back to dashboard if input is missing * Fixing i18n error * Unlink saved search * Fix duplicating embeddable by reference Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: Skip failing test in CI (elastic#75266) [Task Manager] time out work when it overruns in poller (elastic#74980) [Drilldowns] misc improvements & fixes (elastic#75276) Small README note on bumping memory for builds (elastic#75247) [Security Solution][Detections] Adds exception modal tests (elastic#74596) [Dashboard] Sample data link does not work (elastic#75262) [Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable (elastic#74905) [Form lib] Fix issue where serializer on fields are called on every change (elastic#75166) convert processor labels to sentence case (elastic#75278) [Monaco] Refactor the way XJSON grammar checker gets registered (elastic#75160) Clarify no documents error message when filtering by is_training (elastic#75227) [Lens] Fix crash when two layers xychart switches to pie (elastic#75267) [Observability Homepage] Fix console error because of side effect (elastic#75258) [Usage Collection] Add `legacy=true` option to the /api/stats request in the docs (elastic#75146) [ML] Functional tests - re-activate DFA test suites (elastic#75257) GS providers improvements (elastic#75174) [Visualize] First version of by-value visualize editor (elastic#72256)

majagrubic changed the title ~~First version of new by-value editor~~ [Visualize] First version of by-value visualize editor Jul 17, 2020

majagrubic force-pushed the visualize-by-value-editor branch from e693822 to b4972da Compare July 17, 2020 11:29

Maja Grubic added 3 commits July 17, 2020 13:10

Adding missing null check

f5061c7

Making typescript play nicely

8d334c0

Fixing failing tests

6295a08

ThomThomson mentioned this pull request Jul 20, 2020

[Dashboard First] Lens By Value #70272

Closed

7 tasks

majagrubic requested review from ThomThomson, sulemanof and stratoula and removed request for ThomThomson July 20, 2020 20:17

majagrubic marked this pull request as ready for review July 20, 2020 20:20

majagrubic requested a review from a team July 20, 2020 20:20

majagrubic requested a review from a team as a code owner July 20, 2020 20:20

botelastic bot added the Feature:Embedding Embedding content via iFrame label Jul 20, 2020

majagrubic requested a review from Dosant July 20, 2020 20:20

Merge branch 'master' into visualize-by-value-editor

96daa2a

ppisljar approved these changes Jul 21, 2020

View reviewed changes

majagrubic removed the request for review from Dosant July 21, 2020 12:50

sulemanof reviewed Jul 22, 2020

View reviewed changes

ThomThomson mentioned this pull request Jul 30, 2020

Move config setting for By Value Embeddables to Dashboard #73870

Merged

1 task

Merge branch 'master' into visualize-by-value-editor

b63976a

elasticmachine and others added 10 commits August 10, 2020 06:52

Merge branch 'master' into visualize-by-value-editor

db912df

Applying PR comments

8e871ce

Fixing eslint errors

09a8f9b

Fix save as behavior

a201c2a

Fixing HTMLElement type

24c1948

Passing in setOriginatingApp parameter

d4b69dc

Redirect back to dashboard if input is missing

875be62

Fixing i18n error

cc59f71

Merge branch 'master' into visualize-by-value-editor

29c6a31

Unlink saved search

1741aae

majagrubic requested a review from sulemanof August 13, 2020 15:08

ThomThomson reviewed Aug 14, 2020

View reviewed changes

Maja Grubic added 2 commits August 17, 2020 09:22

Merge branch 'master' into visualize-by-value-editor

1259c98

Fix duplicating embeddable by reference

228d007

ThomThomson approved these changes Aug 17, 2020

View reviewed changes

ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 17, 2020

Merged elastic#72256 and elastic#74253 in order to test panel reactiv…

9562949

…ity for by value embeddables

ThomThomson mentioned this pull request Aug 17, 2020

Testing Reactive Issue With Vis By Value #75222

Closed

majagrubic merged commit 9bef317 into elastic:master Aug 18, 2020

majagrubic deleted the visualize-by-value-editor branch August 18, 2020 10:19

majagrubic mentioned this pull request Aug 18, 2020

[7.x] [Visualize] First version of by-value visualize editor (#72256) #75270

Merged

majagrubic mentioned this pull request Aug 19, 2020

[Time to Visualize] Unifying feature flag config for visualize editor and dashboard #75408

Merged

1 task

ThomThomson mentioned this pull request Sep 15, 2020

[Time to Visualize] Lens By Value With AttributeService #77561

Merged

7 tasks

ThomThomson mentioned this pull request Oct 20, 2020

[Time To Visualize] Unify Top Nav Between Lens & Visualize #81246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Visualize] First version of by-value visualize editor #72256

[Visualize] First version of by-value visualize editor #72256

majagrubic commented Jul 17, 2020 •

edited

Loading

elasticmachine commented Jul 20, 2020

majagrubic commented Jul 20, 2020

ppisljar left a comment

sulemanof left a comment •

edited

Loading

sulemanof Jul 22, 2020

majagrubic Jul 22, 2020 •

edited

Loading

sulemanof Jul 22, 2020

sulemanof Jul 22, 2020

majagrubic Jul 22, 2020

mistic commented Aug 10, 2020

majagrubic commented Aug 13, 2020 •

edited

Loading

ThomThomson left a comment

majagrubic commented Aug 17, 2020 •

edited

Loading

kibanamachine commented Aug 17, 2020

ThomThomson left a comment

[Visualize] First version of by-value visualize editor #72256

[Visualize] First version of by-value visualize editor #72256

Conversation

majagrubic commented Jul 17, 2020 • edited Loading

What is this?

Implementation details

What didn't change?

How to test this?

Where are functional/unit tests?

Checklist

For maintainers

elasticmachine commented Jul 20, 2020

majagrubic commented Jul 20, 2020

ppisljar left a comment

Choose a reason for hiding this comment

sulemanof left a comment • edited Loading

Choose a reason for hiding this comment

sulemanof Jul 22, 2020

Choose a reason for hiding this comment

majagrubic Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

sulemanof Jul 22, 2020

Choose a reason for hiding this comment

sulemanof Jul 22, 2020

Choose a reason for hiding this comment

majagrubic Jul 22, 2020

Choose a reason for hiding this comment

mistic commented Aug 10, 2020

majagrubic commented Aug 13, 2020 • edited Loading

ThomThomson left a comment

Choose a reason for hiding this comment

majagrubic commented Aug 17, 2020 • edited Loading

kibanamachine commented Aug 17, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

ThomThomson left a comment

Choose a reason for hiding this comment

majagrubic commented Jul 17, 2020 •

edited

Loading

sulemanof left a comment •

edited

Loading

majagrubic Jul 22, 2020 •

edited

Loading

majagrubic commented Aug 13, 2020 •

edited

Loading

majagrubic commented Aug 17, 2020 •

edited

Loading