Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: Re-rendering visualization when expression changes and improves typing #1491

Merged
merged 3 commits into from
Apr 27, 2022
Merged

Fix: Re-rendering visualization when expression changes and improves typing #1491

merged 3 commits into from
Apr 27, 2022

Conversation

ashwin-pc
Copy link
Member

@ashwin-pc ashwin-pc commented Apr 22, 2022 •
edited
Loading

Description

Changes in this PR:

  • Allows changing visualization expressions in the expressions demo playground. Using different visualization expressions or even editing the existing expression in ReactExpressionRenderer throws an error that uiState cannot be changed for a Visualization. This mitigates that by persisting the uiState for ReactExpressionRenderer for the playground.
  • Improves the types of different render expression functions tied to visualizations.
  • Corrects minor readme styling issues

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@ashwin-pc ashwin-pc requested a review from a team as a code owner April 22, 2022 19:18
@codecov-commenter
Copy link

Codecov Report

Merging #1491 (9d7f85a) into main (376ba8c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1491   +/-   ##
=======================================
  Coverage   68.09%   68.09%           
=======================================
  Files        3072     3072           
  Lines       59032    59032           
  Branches     8928     8928           
=======================================
+ Hits        40198    40200    +2     
+ Misses      16647    16646    -1     
+ Partials     2187     2186    -1
Impacted Files Coverage Δ
...ugins/input_control_vis/public/input_control_fn.ts 100.00% <ø> (ø)
src/plugins/vis_type_table/public/table_vis_fn.ts 100.00% <ø> (ø)
src/plugins/vis_type_vislib/public/pie_fn.ts 100.00% <ø> (ø)
...tions/public/expressions/visualization_function.ts 4.34% <ø> (ø)
src/plugins/visualizations/public/index.ts 0.00% <ø> (ø)
src/plugins/expressions/public/render.ts 80.95% <100.00%> (ø)
...ions/public/expressions/visualization_renderer.tsx 10.00% <100.00%> (ø)
...ic/application/models/sense_editor/sense_editor.ts 64.88% <0.00%> (+0.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 376ba8c...9d7f85a. Read the comment docs.

@tmarkley tmarkley added the visualizations Issues and PRs related to visualizations label Apr 22, 2022
tmarkley
tmarkley approved these changes Apr 22, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great Ashwin!

@ashwin-pc ashwin-pc requested a review from a team April 25, 2022 09:15
boktorbb
boktorbb approved these changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@boktorbb boktorbb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks Ashwin

joshuarrrr
joshuarrrr approved these changes Apr 27, 2022
View reviewed changes
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, and I appreciate the type improvements. A couple minor typing questions.

src/plugins/expressions/common/expression_renderers/types.ts
@@ -79,4 +79,5 @@ export interface IInterpreterRenderHandlers {
reload: () => void;
update: (params: any) => void;
event: (event: any) => void;
uiState?: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - shouldn't this be PersistedState type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could. I wasn't a 100% if uiState was always used as PersistedState everywhere. It is used as PersistedState in the viz renderer but other renderers can use other uiState values. To not break anything I just kept the type that was expected everywhere else. I may update this once I can be sure that it won't break anything.

src/plugins/visualizations/public/types.ts
visData: object;
visConfig: object;
params?: object;
visData?: object | null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - do we need to support null here? Generally undefined is probably preferable and covered by the optional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the existing visualizations expects a null value for vizData when no data is present. Since I didn't want to introduce a breaking change, I kept the expected types the same and instead just typed the interfaces more strongly.

@ashwin-pc ashwin-pc merged commit 1ad1a8b into opensearch-project:main Apr 27, 2022
pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Aug 10, 2022
@ashwin-pc
Fix: Re-rendering visualization when expression changes and improves …
7060be9 
…typing (opensearch-project#1491)

* fix(Expressions): Fixes rendering viz when expression changes

Signed-off-by: Ashwin Pc <[email protected]>

* chore: Improves visualization types

Signed-off-by: Ashwin Pc <[email protected]>

* chore: Updates Render type for other viz expressions

Signed-off-by: Ashwin Pc <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boktorbb boktorbb boktorbb approved these changes

@tmarkley tmarkley tmarkley approved these changes

@joshuarrrr joshuarrrr joshuarrrr approved these changes

Assignees
No one assigned
Labels
visualizations Issues and PRs related to visualizations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ashwin-pc @codecov-commenter @joshuarrrr @tmarkley @boktorbb