[VisBuilder] 2 way communication using UI actions and bug fixes #3732

ashwin-pc · 2023-03-30T03:07:14Z

Description

This PR add 2 way communication to VisBuilder And fixes a few bugs along the way. I've added comments to the section of the code that are bug fixes and not related to the UIActions change.

To add 2 way communication, the existing UI actions framework was reused and the existing actions were simply passed along transparently to the UI. The main task long term is to document and standardize these actions so that a variety of visualization types can use the same actions

Demo:

Screen.Recording.2023-03-30.at.6.49.08.PM.mov

Issues Resolved

Check List

codecov-commenter · 2023-03-30T03:52:30Z

Codecov Report

Merging #3732 (259de2f) into main (971165f) will increase coverage by 0.02%.
The diff coverage is 30.64%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3732      +/-   ##
==========================================
+ Coverage   66.42%   66.44%   +0.02%     
==========================================
  Files        3209     3209              
  Lines       61733    61741       +8     
  Branches     9534     9537       +3     
==========================================
+ Hits        41005    41023      +18     
+ Misses      18441    18429      -12     
- Partials     2287     2289       +2

Flag	Coverage Δ
Linux	`66.38% <30.64%> (+0.02%)`	⬆️
Windows	`66.38% <30.64%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/plugins/vis_builder/public/application/app.tsx	`12.50% <ø> (ø)`
...er/public/application/utils/get_top_nav_config.tsx	`57.77% <ø> (ø)`
...application/utils/use/use_saved_vis_builder_vis.ts	`3.03% <0.00%> (+0.46%)`	⬆️
...ilder/public/embeddable/vis_builder_embeddable.tsx	`1.02% <0.00%> (+0.04%)`	⬆️
...blic/embeddable/vis_builder_embeddable_factory.tsx	`11.11% <0.00%> (ø)`
src/plugins/vis_builder/public/plugin.ts	`22.91% <0.00%> (-0.49%)`	⬇️
...d_visualizations/saved_visualization_references.ts	`0.00% <ø> (ø)`
...uilder/public/application/components/workspace.tsx	`6.45% <25.00%> (+2.88%)`	⬆️
..._builder/public/saved_visualizations/transforms.ts	`80.00% <66.66%> (-20.00%)`	⬇️
...uilder/public/application/components/right_nav.tsx	`12.50% <100.00%> (+3.80%)`	⬆️
... and 2 more

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/plugins/vis_builder/public/application/components/right_nav.tsx

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

joshuarrrr

Looks good! I have a few clarifying questions and comments and a couple of minor requested changes. I still want to play with it a little locally, but my readthrough is done.

CHANGELOG.md

src/plugins/vis_builder/public/saved_visualizations/transforms.test.ts

src/plugins/vis_builder/public/saved_visualizations/transforms.ts

src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx

src/plugins/vis_builder/public/application/components/right_nav.tsx

src/plugins/vis_builder/public/application/utils/handle_vis_event.ts

abbyhu2000 · 2023-04-14T15:37:26Z

src/plugins/vis_builder/public/application/utils/handle_vis_event.ts

+import { VIS_EVENT_TO_TRIGGER } from '../../../../visualizations/public';
+import { UiActionsStart } from '../../../../ui_actions/public';
+
+export const handleVisEvent = async (


Just a question, do we need to add error handling here?

How do see error handling being added here?

Mmm i am mostly thinking if it's possible that await uiActions.getTrigger() fail?

abbyhu2000 · 2023-04-14T15:39:53Z

src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx

+          handleVisEvent(event, getUIActions(), indexPattern.timeFieldName);
+        }
+      })
+    );


Can you explain more why we add the subscriptions to the events here?

The underlying expression renderer may want to transmit user actions to the parent app, it does so by using the expression renderer's event bus. So when the parent application wants to listen to these events, it needs to subscribe to that bus. In the embeddable, the handler.events is that bus, and for ReactExpressionRenderer, the onEvent callback is that bus. In both cases, we want to listen to the event, condition it and trasmit a corresponding UI Action for the event. handleVisEvent is responsible for conditioning the event and transmitting the UIAction

Got it, thanks for the explainations!

abbyhu2000 · 2023-04-14T15:41:43Z

src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx

  // TODO: add indexPatterns as part of configuration
  // indexPatterns?: IIndexPattern[];
  editPath: string;
  editUrl: string;
  editable: boolean;
 }

+export type VisBuilderInput = SavedObjectEmbeddableInput;


nit: should we be more specific on the type name here? VisBuilderEmbeddableInput and VisBuilderEmbeddableOutput?

Think it would be clearer, but it's fine either way.

You are right, that is much more explicit. However, since the UI state PR also affects the same files, i'd avoid making this change now though. Is that okay?

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>

…ent.ts Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

joshuarrrr

LGTM - a couple minor nits and opinions, but nothing blocking.

joshuarrrr · 2023-04-14T23:32:27Z

src/plugins/vis_builder/public/application/components/right_nav.tsx

@@ -121,3 +121,7 @@ const OptionItem = ({ icon, title }: { icon: IconType; title: string }) => (
    <span>{title}</span>
  </>
 );
+
+// The app uses EuiResizableContainer that triggers a rerender for ever mouseover action.


Suggested change

// The app uses EuiResizableContainer that triggers a rerender for ever mouseover action.

// The app uses EuiResizableContainer that triggers a rerender for every mouseover action.

joshuarrrr · 2023-04-14T23:39:41Z

src/plugins/vis_builder/public/application/components/workspace.tsx

@@ -44,15 +46,17 @@ export const Workspace: FC = ({ children }) => {
    async function loadExpression() {
      const schemas = ui.containerConfig.data.schemas;

-      const noAggs = aggConfigs?.aggs?.length === 0;
+      const noAggs = (aggConfigs?.aggs?.length ?? 0) === 0;


opinion - this reads worse to me than

const noAggs = !aggConfigs?.aggs?.length;
or even
const noAggs = aggConfigs?.aggs?.length == false;

Also, for conditionals like this I'd prefer hasNoAggs.

src/plugins/vis_builder/public/application/utils/handle_vis_event.ts

* adds uiActions to visBuilder * prevents multiple errors on load * fixes visbuilder type errors * fixes save * updates changelog --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 1edb195) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

…bug fixes (#3857) * [VisBuilder] 2 way communication using UI actions and bug fixes (#3732) * adds uiActions to visBuilder * prevents multiple errors on load * fixes visbuilder type errors * fixes save * updates changelog --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 1edb195) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md * add changelog Signed-off-by: Josh Romero <rmerqg@amazon.com> --------- Signed-off-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>

…search-project#3732) * adds uiActions to visBuilder * prevents multiple errors on load * fixes visbuilder type errors * fixes save * updates changelog --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> Signed-off-by: David Sinclair <david@sinclair.tech>

ashwin-pc added vis builder backport 2.x v2.7.0 labels Mar 30, 2023

ashwin-pc requested a review from a team as a code owner March 30, 2023 03:07

ashwin-pc self-assigned this Mar 30, 2023

ashwin-pc mentioned this pull request Mar 31, 2023

[VisBuilder] Adds UIState to vis, adds index patterns to embeddable, bug fixes #3751

Merged

8 tasks

ashwin-pc commented Mar 31, 2023

View reviewed changes

src/plugins/vis_builder/public/application/components/right_nav.tsx Show resolved Hide resolved

This was referenced Apr 4, 2023

[CCI] Bug fix for eui select in firefox browser using onclick #3524

Closed

2.7.0 Platform Upgrades for Plugins (O11y, MDS, Geospatial,+) #3674

Closed

ashwin-pc added 5 commits April 5, 2023 17:23

adds uiActions to visBuilder

657d593

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

prevents multiple errors on load

c71fc22

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

fixes visbuilder type errors

ed20b43

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

fixes save

61aeccb

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

updates changelog

f4f2cf0

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

ashwin-pc force-pushed the feature/vis-builder-ui-actions branch from 8cbc8f8 to f4f2cf0 Compare April 5, 2023 17:24

Merge branch 'main' into feature/vis-builder-ui-actions

5c61952

ashwin-pc assigned joshuarrrr and abbyhu2000 Apr 7, 2023

joshuarrrr reviewed Apr 10, 2023

View reviewed changes

abbyhu2000 reviewed Apr 14, 2023

View reviewed changes

Update CHANGELOG.md

a4dbecf

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>

ashwin-pc requested review from ananzh, kavilla, seanneumann, AMoo-Miki, zengyan-amazon, kristenTian, zhongnansu and manasvinibs as code owners April 14, 2023 21:40

ashwin-pc and others added 6 commits April 14, 2023 14:41

Update CHANGELOG.md

cb25986

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>

Update src/plugins/vis_builder/public/application/utils/handle_vis_ev…

c709fbf

…ent.ts Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>

Addresses PR comments

ec7f646

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

Merge branch 'main' into feature/vis-builder-ui-actions

557a2fb

Merge branch 'main' into feature/vis-builder-ui-actions

bac1c9f

Fix changelog

259de2f

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

joshuarrrr approved these changes Apr 14, 2023

View reviewed changes

abbyhu2000 approved these changes Apr 17, 2023

View reviewed changes

joshuarrrr merged commit 1edb195 into opensearch-project:main Apr 17, 2023

opensearch-trigger-bot bot mentioned this pull request Apr 17, 2023

[Backport 2.x] [VisBuilder] 2 way communication using UI actions and bug fixes #3857

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VisBuilder] 2 way communication using UI actions and bug fixes #3732

[VisBuilder] 2 way communication using UI actions and bug fixes #3732

ashwin-pc commented Mar 30, 2023 •

edited

Loading

codecov-commenter commented Mar 30, 2023 •

edited

Loading

joshuarrrr left a comment

abbyhu2000 Apr 14, 2023

ashwin-pc Apr 14, 2023

abbyhu2000 Apr 17, 2023

abbyhu2000 Apr 14, 2023

ashwin-pc Apr 14, 2023

abbyhu2000 Apr 17, 2023

abbyhu2000 Apr 14, 2023

ashwin-pc Apr 14, 2023

joshuarrrr left a comment

joshuarrrr Apr 14, 2023

joshuarrrr Apr 14, 2023

	// The app uses EuiResizableContainer that triggers a rerender for ever mouseover action.
	// The app uses EuiResizableContainer that triggers a rerender for every mouseover action.

[VisBuilder] 2 way communication using UI actions and bug fixes #3732

[VisBuilder] 2 way communication using UI actions and bug fixes #3732

Conversation

ashwin-pc commented Mar 30, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Mar 30, 2023 • edited Loading

Codecov Report

joshuarrrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuarrrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc commented Mar 30, 2023 •

edited

Loading

codecov-commenter commented Mar 30, 2023 •

edited

Loading