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

feat(xy): mutilayer time axis step 1 #1326

Merged
merged 76 commits into from
Sep 8, 2021

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Aug 24, 2021

Summary

Adds a preview of the hierarchical time axis via a new multiaxis story, including the minor code update to realize the preexisting left-align specification for the horizontal axis ticks. Refactors and improves on a lot of the Cartesian axis related code

image

BREAKING CHANGE

  • feat: removes the axis deduplication feature
  • fix: showDuplicatedTicks now causes a duplication check on the actual axis tick label (possibly yielded by tickFormat rather than the more general labelFormat)

Details

  • adds a hierarchical time axis story
  • fix/feat: the small update to actually left-align the horizontal axis ticks
  • BREAKING feat: removes the axis deduplication feature
  • BREAKING fix: showDuplicatedTicks causes a duplication check on the actual axis tick label (possibly yielded by Axis.tickLabel rather than the more general tickFormat)
  • fix: turned all [].sort() calls safe (some API inputs used to be modified in the past; also, brittle to future code changes)
  • fix(test): makes certain array comparison tests use toIncludeSameMembers instead of toEqual so the test remain neutral about the order of array elements (not all arrays need to have a specific order)
  • fix: axisTickLabel (populated by Axis.labelFormat) is now precomputed and used for bounding box calculation and wherever it's needed for the actual axis tick rendering, as the preexisting label (populated by Axis.tickFormat) was generic, used eg. for tooltip formatting
  • test: custom font that's fairly usable in live storybook mode (font for a11y: Atkinson Hyperlegible)
  • refactor: swathes of axis related code eg. 350 lines removed from axis_utils.ts alone, not counting the deletion of the axis dedupe fun (refactoring is guided by certain objectives such as clarity of data flow, performance, simple types, avoidance of special cases and bails, expressions instead of control structures, succinctness)

Issues

Step 1 toward #1310 and elastic/kibana#51227

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)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • 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 marked this pull request as draft August 24, 2021 09:34
@monfera
Copy link
Contributor Author

monfera commented Aug 24, 2021

jenkins test this

@monfera monfera force-pushed the crowded-bar-labels-01 branch 6 times, most recently from 6f86601 to 70a5f4e Compare August 30, 2021 10:21
@monfera monfera changed the title feat(xy): labels for crowded bars 01 test: draft PR for CI runs Aug 30, 2021
@monfera monfera force-pushed the crowded-bar-labels-01 branch from 1955042 to ac4f54e Compare September 2, 2021 22:15
@markov00 markov00 mentioned this pull request Sep 7, 2021
@monfera monfera changed the title test: draft PR for CI runs feat: mutilayer time axis steo 1 Sep 7, 2021
@monfera monfera changed the title feat: mutilayer time axis steo 1 feat(xy): mutilayer time axis steo 1 Sep 7, 2021
@monfera monfera added :refactor Code refactor :xy Bar/Line/Area chart related code quality enhancement New feature or request :axis Axis related issue labels Sep 7, 2021
@monfera monfera changed the title feat(xy): mutilayer time axis steo 1 feat(xy): mutilayer time axis step 1 Sep 7, 2021
@monfera monfera force-pushed the crowded-bar-labels-01 branch from 61c5507 to 5a587c5 Compare September 7, 2021 23:21
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. Didn't have time yet to go through the axis diff in great detail.

I think we should have a replacement VRT for every test that was deleted before merging this PR.

@@ -526,53 +319,35 @@ export function enableDuplicatedTicks(
fallBackTickFormatter: TickFormatter,
tickFormatOptions?: TickFormatterOptions,
): AxisTick[] {
const ticks = scale.ticks();
const allTicks: AxisTick[] = ticks.map((tick) => ({
const allTicks: AxisTick[] = scale.ticks().map((tick) => ({
value: tick,
// TODO handle empty string tick formatting
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure. I think that was just to handle someone passing () => '' to the tickFormat.


if (!axisSpec) {
return acc;
if (axisSpec) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I'm not sold early return is best either. I don't see any performance pros and cons. But some times the fall-through case is usefully to where you don't have to do something like...

const myFunc(value: any) {
  if (!value) return false;
  if (value % 2) return true;
  return false;
};

I know it is in the kibana style guide but I don't love it.

packages/charts/src/common/color_calcs.ts Show resolved Hide resolved
storybook/style.scss Show resolved Hide resolved
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.

I've tested it locally and works great (with the various fixed to the tickFormatter/labelFormatter now the tooltip header works great)

I think is good to merge after solving the legit Nick's comments

@monfera
Copy link
Contributor Author

monfera commented Sep 8, 2021

@nickofthyme @markov00

I think we should have a replacement VRT for every test that was deleted

My thought as well, to lock in the outcomes of the logic incl. corner case handling whose unit tests were removed. It's critical to aim for an as high ratio of code coverage with VRT as possible, hopefully 100% over time, at least everything that ultimately influences what gets rendered statically or upon interaction

before merging this PR

Due to the high volume of code that's changed laboriously and non-automatically, merging this soon would avoid nasty merge conflicts, and the much more limited (merge conflict free) test updates could be an immediate follow-up. Besides the merge conflict angst, the PR contains a few fixes (and VRT's them) and added safety so the balance may be positive. An earlier merge brings up potential regressions sooner. My recent stint with refactoring PRs removed around a couple thousand runtime lines and changed another few thousands, and where there's change there's risk of regression, so being exposed to usage feedback earlier has some merit.

Whether you prefer merging sooner or you'd rather await VRT coverage, I'd like to get help from the two of you, as sometimes it's not immediately clear what a unit test meant to lock in, in terms of user intent to final results. As you have more elastic-charts experience with the Cartesians, and wrote the tests, it's also more efficient to go parallel on this, or at least walk me through this on a call so we gather what images can cover the unit tests that got disused for one reason or another

Also linking Marco's "one for the team" PR as it covers some of what went away in unit tests here

@nickofthyme
Copy link
Collaborator

@monfera sure that's understandable. I think we just need to add some some storybook tests to replicate some of the unit test setup.

@monfera monfera merged commit 867b1f5 into elastic:master Sep 8, 2021
nickofthyme pushed a commit that referenced this pull request Sep 13, 2021
# [35.0.0](v34.2.1...v35.0.0) (2021-09-13)

### Bug Fixes

* **a11y:** restore focus after popover close with color picker ([#1272](#1272)) ([0c6f945](0c6f945)), closes [#1266](#1266) [#935](#935)
* **build:** fix license in package.json ([#1362](#1362)) ([d524fdf](d524fdf))
* **deps:** update dependency @elastic/eui to ^37.5.0 ([#1341](#1341)) ([fb05c98](fb05c98))
* **deps:** update dependency @elastic/eui to ^37.6.1 ([#1359](#1359)) ([2ae90ce](2ae90ce))
* **deps:** update dependency @elastic/eui to ^37.7.0 ([#1373](#1373)) ([553b6b0](553b6b0))
* **heatmap:** filter out tooltip picked shapes in x-axis area ([#1351](#1351)) ([174047d](174047d)), closes [#1215](#1215)
* **heatmap:** remove values when brushing only over axes ([#1364](#1364)) ([77ff8d3](77ff8d3))

### Features

* **annotations:** add onClickHandler for annotations ([#1293](#1293)) ([48198be](48198be)), closes [#1211](#1211)
* **heatmap:** add text color contrast to heatmap cells ([#1342](#1342)) ([f9a26ef](f9a26ef)), closes [#1296](#1296)
* **heatmap:** reduce font size to fit label within cells ([#1352](#1352)) ([16b5546](16b5546))
* **xy:** mutilayer time axis step 1 ([#1326](#1326)) ([867b1f5](867b1f5))

### BREAKING CHANGES

* **xy:** - feat: removes the axis deduplication feature
- fix: `showDuplicatedTicks` causes a duplication check on the actual axis tick label (possibly yielded by `Axis.tickLabel` rather than the more general `tickFormat`)
* **heatmap:** the `config.label.fontSize` prop is replaced by `config.label.minFontSize` and `config.label.maxFontSize`. You can specify the same value for both properties to have a fixed font size. The `config.label.align` and `config.label.baseline` props are removed from the `HeatmapConfig` object.
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 35.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:axis Axis related issue code quality enhancement New feature or request :refactor Code refactor released Issue released publicly :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants