Skip to content

Conversation

@monfera
Copy link
Contributor

@monfera monfera commented Aug 18, 2022

Summary

  • Collapses time into a single hh:mm[:ss] layer
  • The Timeslip alpha chart could therefore switch to a maximum of two labeled time axis layers (suggested for all other charts)
  • The minimum gridline pitch for a new unlabeled grid layer to appear is about 25% wider, so the chart/grid is slightly less dense
  • Added the weekly (week start) unlabeled raster into the starting raster set (see new gridlines with dailies)
  • Made the weekly labels (day of the start of the week) more spaced out to reduce their horizontal density
  • Switched to long month names for all resolutions in the bottom (context) labeled layer for consistency
  • Timeslip alpha: Deeper maximum zoom so normal sized charts can reach the individually labeled milliseconds level
  • Factored out numerous common constants, formatters and layer cascading (object spreads) for more code DRYness and easier future maintenance
  • Minor type and other code improvements

image

Details

Issues

Closes #1777

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@monfera monfera added :axis Axis related issue :xy Bar/Line/Area chart related labels Aug 18, 2022
@monfera
Copy link
Contributor Author

monfera commented Aug 18, 2022

buildkite update vrt

@markov00
Copy link
Member

Hi @monfera it looks great in my opinion. I'm wondering if we can add intermediate steps between the jumps, for example:

  • between days and 6-hours, add 12 hours raster
  • between 6-hour and hour, add a 3 hours raster
  • between hourly and quarter hours, add a half-hour raster
  • between minutes and quarter minutes, add an half-minute raster

There could be a bit more raster to add, but can be considered to add a more "sparse" representation.
Thanks to the already existing unlabelled raster, understanding the time in between two labelled rasters is easy and you have to add up to 5 to one date/time to get the right value, looks simple enough for everybody and also reduces a but he labelling noise around, WDYT?

@monfera
Copy link
Contributor Author

monfera commented Aug 22, 2022

@markov00 I think some or all the rasters can be added, but it'd be great to discuss and implement it in a next PR, as it's in my understanding, lower priority than, and separable from the merger of the layers:

  • The full absorption of this current PR will involve a change from 3 to 2 layers. So it'd be useful to get a head start on that. so it's available for developers using charts
  • From a risk management PoV it's nice to decouple the two PRs
  • The addition of new rasters doesn't fundamentally change the overall logic such as collapsing and labeled layer count, so the next PR would be a more incremental one. As we have no user feedback for that, there's less of a time pressure for trying them (deployed in a new PR) and discussing them from a design PoV

@monfera
Copy link
Contributor Author

monfera commented Aug 22, 2022

@markov00 btw. I was planning to update the 3-layer stories to 2-layer stories in a separate PR too, so the effect of individual changes can be more incrementally assessed, but I can add it to this PR if you think it's better that way

@markov00
Copy link
Member

markov00 commented Aug 22, 2022

@monfera can we say that we are going to deprecate completely the 3 layers implementation? or do you like to keep that configuration and let the developers decide?

@monfera
Copy link
Contributor Author

monfera commented Aug 22, 2022

@markov00 Yes I think it's best to switch from where it is 3 layers today to 2 layers, because the reason for the 3 layers was the rather granular layer labeling, which we consolidated with the layer collapse. You might want to circulate it among call sites and PMs, though it's not in the way of this specific PR, as layer count is externally specified

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

It looks great to me.
I've added just one comment to check before merging

markov00 and others added 8 commits August 23, 2022 14:51
This PR fixes the problem caused by a wrongly generated React key on each grid items.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Marco Vettorello <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* fix(metric): clip title at right length

* test(vrt): update screenshots [skip ci]

Co-authored-by: elastic-datavis[bot] <98618603+elastic-datavis[bot]@users.noreply.github.com>
## [47.2.1](elastic/elastic-charts@v47.2.0...v47.2.1) (2022-08-23)

### Bug Fixes

* **deps:** update dependency @elastic/eui to v63 ([elastic#1783](elastic#1783)) ([554f370](elastic@554f370))
* **deps:** update dependency @elastic/eui to v64 ([elastic#1798](elastic#1798)) ([bb028e9](elastic@bb028e9))
* **metric:** clip title at right length ([elastic#1790](elastic#1790)) ([7d7ad55](elastic@7d7ad55))
* **metric:** use a correct React key for each grid item ([elastic#1789](elastic#1789)) ([076406e](elastic@076406e))
@monfera monfera enabled auto-merge (squash) August 23, 2022 13:17
@monfera monfera merged commit 577cfee into elastic:master Aug 23, 2022
nickofthyme pushed a commit that referenced this pull request Aug 31, 2022
# [48.0.0](v47.2.1...v48.0.0) (2022-08-31)

### Bug Fixes

* **deps:** update dependency @elastic/eui to v65 ([#1802](#1802)) ([37a0ce7](37a0ce7))
* **deps:** upgrade uuid to latest version ([#1794](#1794)) ([35bb737](35bb737))

### Features

* **xy:** collapsing time into a single hh:mm[:ss] layer ([#1791](#1791)) ([577cfee](577cfee))

### BREAKING CHANGES

* **xy:** show hh:mm in a single labeled axis layer and other grid related adjustments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:axis Axis related issue :xy Bar/Line/Area chart related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiline time axis: collapsing some of the layers

3 participants