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

Create vis_type_xy plugin to replace histogram, area and line charts #78154

Merged
merged 158 commits into from
Dec 18, 2020

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Sep 22, 2020

Summary

Release Notes: Use elastic chart library for bar/area/line charts in Visualize

Adds new implementation for area, line and histogram vis types using @elastic/charts in visualize.

closes #77186 closes #63531 closes #51553 closes #39061 closes #29147 closes #13093 closes #12999 closes #4215 closes #59076 closes #41887 closes #43251 closes #45602

Future tasks:

Problematic case:

  • using back button on this vis causes error with minInterval link
  • axis disappear when assigned metric is hidden link
  • using min value greater than max of data throws error link

UI changes to current vislib (flag disabled)

  • Ability to change any axis position to enable rotation. Before the y (aka values) axes were blocked to horizontal/vertical depending on the x (aka category) axis position
  • Show option for Show values on chart only when bar chart exists

UI changes to new (flag enabled)

  • Show detailed tooltip to match the current vislib tooltip
  • Allow fitting functions for line and area charts to fill null/missing values
  • New eui color palette
  • Hovering on chart element highlights hovered element and not the whole series like current vislib

UI changes to discover

  • Allow brushing final bucket, even beyond the end domain range. Currently brushing beyond will just set the value to the start of the last bucket.

Other notable changes:

  • Moves vis editor components, collections and types to vis_type_xy plugin
  • Moves interval and endzones logic and components from discover to charts plugin
  • Moves current time logic from discover to charts plugin
  • Moves vislib plugin expression functions from build_pipeline to vis_type_vislib plugin
  • Moves vislib vis type definitions for histogram, horizontal_bar, area and line vis types into vis_type_xy plugin and referenced from vis_type_vislib plugin.
  • Adds vis migration for area, line and histogram vis types to add the properties isVislibChart and detailedTooltip, used for backwards compatibility with vislib.

Checklist

Release Notes

Replaces legacy visualize chart library with @elastic/charts. Toggled via advanced settings.

- move editor config components from vislib to xy plugin
- import editor values from xy to vislib
- move vislib type definitions to xy plugin and reference from vislib
- refactor types to be consistent with elastic-charts
- add toExpression method on visType
- move vislib expression build into vislib plugin
- update vis types for toExpressionAst method
- add color picker component
- remove unused methods from MappedColors class
- add legend toggle component
- add elastic charts click transforms for filter and brush events
- add detailed tooltip option to render vislib style tooltip
- add fitting functions for area and line charts
- fix endzones to work with detailed tooltip
- make detailed tooltip default for all old vislib types
@nickofthyme nickofthyme linked an issue Sep 28, 2020 that may be closed by this pull request
- add extra loop with newChartUI enabled
- update visChart pageObject to conform to either vislib or elastic charts
- update visEditor pageObject to conform to either vislib or elastic charts
- update services to account for changes from xy plugin and typescriptification
- convert all visualize functional tests to typescript
- update tests to use either vislib or elastic charts
- add loop for newChartUI tests
- refactor series filter logic to allow fn accessors
- cleanup filter logic to reuse code
- fix filters on _all buckets with no x metric
@nickofthyme
Copy link
Contributor Author

Thanks @nickofthyme , clicking ranges works now. However I think there's still some bug in there - if ranges are used as split series it just says "No data" - using vislib it renders fine:
Screenshot 2020-12-08 at 12 12 49
Screenshot 2020-12-08 at 12 12 32

Sorry for the delay but this required changes in elastic charts to work. See changes in 8e7b5d0

Now when using ranges as split series with no x metric you will see something like...

Screen Recording 2020-12-16 at 05 40 PM

Also when using ranges as split series with range x metric you will see something like...

Screen Recording 2020-12-16 at 05 43 PM

All filters are working, still shows duplicated per #66595

@stratoula
Copy link
Contributor

@nickofthyme I am going to review this until the end of the week. Just FYI, there is this PR which if it is merged you may need to do some small adjustments on yours.

@stratoula
Copy link
Contributor

stratoula commented Dec 17, 2020

The split series with range indeed works now. But when I add Date Histogram on x-axis I see a blank chart (no error on the console). Same configuration with vislib works. It seems to fail only with Date Histogram.

vislib
Screenshot 2020-12-17 at 4 22 28 PM

es-charts
Screenshot 2020-12-17 at 4 22 51 PM

I can also replicate it when I add terms on split series.
Screenshot 2020-12-17 at 4 28 55 PM

@nickofthyme
Copy link
Contributor Author

The split series with range indeed works now. But when I add Date Histogram on x-axis I see a blank chart (no error on the console). Same configuration with vislib works. It seems to fail only with Date Histogram.

@stratoula Good catch! See 6417be1

Fixed...

Image 2020-12-17 at 11 59 12 AM

...and fixed...

Image 2020-12-17 at 11 59 47 AM

@nickofthyme
Copy link
Contributor Author

nickofthyme commented Dec 17, 2020

@ryankeairns Could you approve this as the @elastic/kibana-core-ui member? Please and thank you.

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

LGTM! I did a final test and I can't find any more bugs! Thank you @nickofthyme, great work ❤️

migrationVersion: {
visualization: '7.11.0',
},
migrationVersion: resp.body.saved_objects[0].migrationVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

yay 🎉

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

The CSS changes look good (and very minor). I see there have been some adjustments for things like spacing, padding, etc., so I'll make a note to do another visual pass (post-merge) and touch up any remaining small bits.

Great work!

@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 16, 2021

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [755dc7e]

History

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

@nickofthyme nickofthyme restored the vislib-replacement-pt-1 branch September 1, 2021 14:13
@nickofthyme nickofthyme deleted the vislib-replacement-pt-1 branch November 18, 2021 14:35
@nickofthyme nickofthyme restored the vislib-replacement-pt-1 branch November 18, 2021 14:43
@nickofthyme nickofthyme deleted the vislib-replacement-pt-1 branch November 18, 2021 14:45
@nickofthyme nickofthyme restored the vislib-replacement-pt-1 branch November 18, 2021 14:46
@nickofthyme nickofthyme deleted the vislib-replacement-pt-1 branch November 18, 2021 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment