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.

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

[Vis Builder] Update vislib params and misc fixes #2610

Merged
merged 3 commits into from
Oct 19, 2022

Conversation

ashwin-pc
Copy link
Member

@ashwin-pc ashwin-pc commented Oct 19, 2022

Description

This PR consists of 2 main changes

  • Fixes for issues identified from Functional tests (commit: 563342)
  • Adds additional aggregation parameters for Vislib charts (Bar, Line and Area) (Commit: d96669)

Issues Resolved

Check List

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

@ashwin-pc ashwin-pc requested a review from a team as a code owner October 19, 2022 01:19
@ashwin-pc ashwin-pc changed the title Update vis params [VisBuilder] Update vis params and misc fixes Oct 19, 2022
@ashwin-pc ashwin-pc changed the title [VisBuilder] Update vis params and misc fixes [Vis Builder] Update vislib params and misc fixes Oct 19, 2022
Signed-off-by: Ashwin P Chandran <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #2610 (563342b) into main (f29f734) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 563342b differs from pull request most recent head f92ec36. Consider uploading reports for the commit f92ec36 to get more accurate results

@@            Coverage Diff             @@
##             main    #2610      +/-   ##
==========================================
- Coverage   66.81%   66.80%   -0.02%     
==========================================
  Files        3207     3207              
  Lines       61136    61137       +1     
  Branches     9313     9313              
==========================================
- Hits        40851    40845       -6     
- Misses      18055    18060       +5     
- Partials     2230     2232       +2     
Impacted Files Coverage Δ
...ilder/public/embeddable/disabled_visualization.tsx 0.00% <ø> (ø)
...ilder/public/embeddable/vis_builder_embeddable.tsx 0.99% <0.00%> (-0.01%) ⬇️
...blic/embeddable/vis_builder_embeddable_factory.tsx 11.11% <ø> (ø)
...public/visualizations/vislib/area/area_vis_type.ts 100.00% <ø> (ø)
...ualizations/vislib/histogram/histogram_vis_type.ts 100.00% <ø> (ø)
...public/visualizations/vislib/line/line_vis_type.ts 100.00% <ø> (ø)
...s/osd-optimizer/src/node/node_auto_tranpilation.ts 83.67% <0.00%> (-4.09%) ⬇️
packages/osd-optimizer/src/node/cache.ts 50.00% <0.00%> (-2.78%) ⬇️
...ic/application/models/sense_editor/sense_editor.ts 64.00% <0.00%> (-0.89%) ⬇️

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

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.

Looks good.

  1. Why did we need to add uiState to the embeddable?
  2. For the future, we should consider consolidating common UI schemas (line, area, histogram and other XY-type charts share a bunch of properties that we can dedupe and inherit)

group: AggGroupNames.Buckets,
name: 'group',
title: i18n.translate('visTypeVislib.area.groupTitle', {
defaultMessage: 'Split series',
Copy link
Member

Choose a reason for hiding this comment

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

nice, I was going to ask you about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, was called out in user testing :)

(result: any) => isLabsEnabled || result.type?.stage !== 'experimental'
(result: any) =>
isLabsEnabled ||
(result.type?.stage !== 'experimental' && result.stage !== 'experimental')
Copy link
Member

Choose a reason for hiding this comment

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

this seems a little weird... maybe a comment would help explain?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because depending on the type of visualization (using the 'visualization' saved object or 'visBuilder' visualization) the location of the experimental property changes. I'd add a comment but honestly why they are in two different locations is beyond me so a comment would essentially describe this as you see it in the code. We dont have other aliased visualization to know for sure if its specific to aliased visualizations or just VisBuilder

@ashwin-pc
Copy link
Member Author

Why did we need to add uiState to the embeddable?

Vislib visualization expect the UIState object to persist even when the parameters for the visualization change. We dont use the UIState in the visualization yet so i'm just passing in an empty UIState for now. Without this, updating the query in the Dashboard will cause VisLib visualizations to error out.

For the future, we should consider consolidating common UI schemas (line, area, histogram and other XY-type charts share a bunch of properties that we can dedupe and inherit)

Agreed. I didnt do that here because of the upcoming transition away from VisLib. This view keeps understanding each vis types properties a lot easier. Can go either way, but this way its easier to reason them when we refactor.

@ashwin-pc ashwin-pc merged commit 33aed0a into opensearch-project:main Oct 19, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 20, 2022
* Updates bar, line, area visbuilder config

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

* Misc fixes from functional testing

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

* Adds Changelog entry

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

Signed-off-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit 33aed0a)
kavilla pushed a commit that referenced this pull request Oct 20, 2022
* Updates bar, line, area visbuilder config

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

* Misc fixes from functional testing

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

* Adds Changelog entry

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

Signed-off-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit 33aed0a)
vimalMK pushed a commit to vimalMK/OpenSearch-Dashboards that referenced this pull request Oct 20, 2022
…#2610)

* Updates bar, line, area visbuilder config

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

* Misc fixes from functional testing

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

* Adds Changelog entry

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

Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: vimal k <[email protected]>
@AMoo-Miki AMoo-Miki added test:functional v2.4.0 'Issues and PRs related to version v2.4.0' labels Nov 5, 2022
pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Dec 1, 2022
…#2610) (opensearch-project#2630)

* Updates bar, line, area visbuilder config

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

* Misc fixes from functional testing

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

* Adds Changelog entry

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

Signed-off-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit 33aed0a)
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
…#2610)

* Updates bar, line, area visbuilder config

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

* Misc fixes from functional testing

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

* Adds Changelog entry

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

Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x test:functional v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants