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

[charts] Fix multilayer time axis styles #116749

Merged
merged 15 commits into from
Nov 3, 2021
Merged

[charts] Fix multilayer time axis styles #116749

merged 15 commits into from
Nov 3, 2021

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Oct 29, 2021 •
edited
Loading

Summary

This PR fixes a set of issues to the new multilayer time axis:

  • the tickLine is now removed from ticks without a label
  • the axis/tick/label style is restored to the original EUI one
  • the multilayer time axis style is now moved into the charts plugin and reused
  • Lens: use the single-layer time axis when bars cluster is used
  • TSVB: I reduced a bit the number of ticks on the Y axis, to reduce the noise of gridlines with multilayer axis
  • Discover: I reduced by 8px the height of the histogram and moved the top padding to the bottom to separate a bit the time range text and the time axis

Coming from @elastic/charts update:

@markov00 markov00 changed the title 2021 10 28 fix multitime axis styles [charts] Fixe multilayer time axis styles Oct 29, 2021
@markov00 markov00 changed the title [charts] Fixe multilayer time axis styles [charts] Fix multilayer time axis styles Oct 29, 2021
@markov00 markov00 added auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience Feature:ElasticCharts Issues related to the elastic-charts library release_note:skip Skip the PR/issue when compiling release notes Team:DataVis Team label for DataVis Team v8.0.0 v8.1.0 labels Oct 29, 2021
@brianseeders brianseeders changed the base branch from main to master October 29, 2021 15:16
@markov00
Copy link
Member Author

markov00 commented Nov 1, 2021

@elasticmachine merge upstream

@markov00 markov00 mentioned this pull request Nov 1, 2021
Open
11 tasks
@markov00 markov00 marked this pull request as ready for review November 1, 2021 17:08
@markov00 markov00 requested review from a team as code owners November 1, 2021 17:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/datavis (Feature:ElasticCharts)

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Nov 1, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

dominiqueclarke
dominiqueclarke approved these changes Nov 1, 2021
View reviewed changes
Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

Uptime changes LGTM

kertal
kertal approved these changes Nov 2, 2021
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Data Discover team owned code LGTM; tested Discover a-la-carte, works as expected
Discover - Elastic 2021-11-02 at 2 16 48 PM

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

stratoula
stratoula reviewed Nov 3, 2021
View reviewed changes
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.

It looks fine, the only thing I noticed is that a TSVB area chart doesn't have the new axis.
image

Moreover, we could also enable the timeAxis for Timelion (irrelevant with this PR but just a thought). Wdyt?

@stratoula
Copy link
Contributor

Another thing I noticed is that the labels orientation doesn't work for the new axis
image

Maybe it makes sense to disable them?

@markov00
Copy link
Member Author

markov00 commented Nov 3, 2021 •
edited
Loading

It looks fine, the only thing I noticed is that a TSVB area chart doesn't have the new axis.

You are right, I've pushed a fix here 6bcad3f

Moreover, we could also enable the timeAxis for Timelion (irrelevant with this PR but just a thought). Wdyt?

Not right now and not for 8.0, the timelion default parameters for bars are not suitable to be used with the multilayer time axis. We can enable it only for area/line charts in case. We are anyway after FF and we can't merge new features in actually.

Another thing I noticed is that the labels orientation doesn't work for the new axis !
Maybe it makes sense to disable them?

We are going to disable them but I prefer to disable them in a different PR, this one should be merged first as it is way more important for now

@markov00 markov00 mentioned this pull request Nov 3, 2021
Merged
stratoula
stratoula approved these changes Nov 3, 2021
View reviewed changes
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.

@markov00 I totally agree. About timelion I was thinking to enable it on a future minor if it makes sense and I am fine to disable them on another PR, it is mostly for UX reasons.

Changes LGTM! I tested it locally and works fine in Lens, TSVB and. Visualize

@markov00 markov00 enabled auto-merge (squash) November 3, 2021 11:00
@markov00 markov00 merged commit 5fc70f5 into elastic:main Nov 3, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
charts 64 66 +2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
charts 254 278 +24

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 303.5KB 303.2KB -373.0B
lens 953.9KB 953.8KB -137.0B
visTypeTimeseries 443.7KB 443.4KB -259.0B
visTypeXy 59.7KB 59.6KB -100.0B
total -869.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
charts 56.7KB 57.1KB +395.0B
kbnUiSharedDeps-npmDll 5.0MB 5.0MB -79.0B
total +316.0B
Unknown metric groups

API count

id before after diff
charts 286 310 +24

History

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 3, 2021
@markov00 @kibanamachine
[charts] Fix multilayer time axis styles (elastic#116749)
d0edf6f 
This commit fixes a set of issues to the new multilayer time axis:
- the tickLine is now removed from ticks without a label
- the axis/tick/label style is restored to the original EUI one
- the multilayer time axis style is now moved into the charts plugin and reused
- Lens: use the single-layer time axis when bars cluster is used
- TSVB: I reduced a bit the number of ticks on the Y axis, to reduce the noise of gridlines with multilayer axis
- Discover: I reduced by 8px the height of the histogram and moved the top padding to the bottom to separate a bit the time range text and the time axis

Coming from @elastic/charts update:
- multilayer time axis tick/grid is shown only when tick is inside domain (this removes the black vertical axis line at the beginning of the chart) fix(xy): show mouse cursors on charts with opaque background elastic-charts#1447
- Fix the invisible cursor on charts with opaque backgrounds fix(xy): multilayer time axis tick/grid only when tick is inside domain elastic-charts#1446
- Add missing last tick and rarify gridlines fix(xy): adding missing last tick and other tick improvements elastic-charts#1448
@kibanamachine kibanamachine mentioned this pull request Nov 3, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 3, 2021
@kibanamachine @markov00
[charts] Fix multilayer time axis styles (#116749) (#117296)
3a73987 
This commit fixes a set of issues to the new multilayer time axis:
- the tickLine is now removed from ticks without a label
- the axis/tick/label style is restored to the original EUI one
- the multilayer time axis style is now moved into the charts plugin and reused
- Lens: use the single-layer time axis when bars cluster is used
- TSVB: I reduced a bit the number of ticks on the Y axis, to reduce the noise of gridlines with multilayer axis
- Discover: I reduced by 8px the height of the histogram and moved the top padding to the bottom to separate a bit the time range text and the time axis

Coming from @elastic/charts update:
- multilayer time axis tick/grid is shown only when tick is inside domain (this removes the black vertical axis line at the beginning of the chart) fix(xy): show mouse cursors on charts with opaque background elastic-charts#1447
- Fix the invisible cursor on charts with opaque backgrounds fix(xy): multilayer time axis tick/grid only when tick is inside domain elastic-charts#1446
- Add missing last tick and rarify gridlines fix(xy): adding missing last tick and other tick improvements elastic-charts#1448

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
markov00 added a commit to nickofthyme/kibana that referenced this pull request Dec 20, 2021
@markov00
Cherry-pick tests fixes from PR elastic#116749 and elastic#115853
20dd1c3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

@dominiqueclarke dominiqueclarke dominiqueclarke approved these changes

@kertal kertal kertal approved these changes

@ryankeairns ryankeairns ryankeairns approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience Feature:ElasticCharts Issues related to the elastic-charts library release_note:skip Skip the PR/issue when compiling release notes Team:DataVis Team label for DataVis Team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.0.0 v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@markov00 @elasticmachine @stratoula @kibanamachine @ryankeairns @kertal @dominiqueclarke