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

fix(axes): start of week label on multilayer time axis #2035

Merged
merged 13 commits into from
May 23, 2023

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Apr 27, 2023

Summary

This PR resolves confusion around the start day of week labeling near the end of the month.

Before

Screen Recording 2023-04-27 at 09 58 27 AM

After

Screen Recording 2023-04-27 at 09 53 41 AM

Details

Currently the raster layer intervals are computed using a lower bound (i.e. minimum) and upper bound (i.e. supremum). This supremum value is used, in part, to determine how long we stick the axis label before clipping it off the screen. In the case of this issue, the start day of the week sticks past the given month into the next creating confusion.

This PR is a partial solution to clamp the start day supremum between the binStart and the end of the given month. This solution works fine for static timeslip axes but when looking at the dynamic timeslip chart, we may run into issues where the tick label is longer than the available space, causing strange positioning with disappearing text.

With this fix we also lose the context of the previous interval bin start. A future improvement of this logic is described in #1934 (comment).

Implementation changes

This PR adds a new property to the Interval type called labelSupremum. This property is used as a false supremum only for purposes of label text placement. Changing the logic for the supremum would have created a conflict between the actual supremum and that used only for text resulting in a broken interval logic and scale marker logic.

Issues

Fix for elastic/kibana#148872

Partially solves #1934

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

@nickofthyme nickofthyme added :axis Axis related issue :xy Bar/Line/Area chart related :timeslip Timeslip related issues labels Apr 27, 2023
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 like the logic of adding an additional prop to handle the label space.
The logic needs to be fixed also on the xy_chart (probably already in progress locally, but not written in the TODO list)
For the xy_chart configuration, you could use the Area->Timeslip storybook example, and change the knobs configurations (like in the attached screenshot) to get the March 28th 2022 label that overlaps the April 2022. The 28th label should be removed by this fix.

Screenshot 2023-04-28 at 10 10 25

Screenshot 2023-04-28 at 09 57 26

Screenshot 2023-04-28 at 10 12 23

@nickofthyme
Copy link
Collaborator Author

nickofthyme commented Apr 30, 2023

@markov00 Thanks, I fixed the xy_chart issue in 266b457. It's strange to me that only the continuousTimeRasters is shared between the timeslip and the xy_chart axis implementation, where there appears to be a bit of duplicate code higher in the render path. Maybe this could be improved in the future.

image

@nickofthyme nickofthyme requested a review from markov00 May 1, 2023 20:35
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.

Changes look good but I have a hard time understanding why in some stories the 2 layers become a single layer (with overlapping text).
Also, this doesn't looks great axis-stories/uses-proper-english-ending-at-a-weekly-resolution.png because it seems that we reduced the frequency of the unlabelled months from 1 line every month to one line every 4 months

@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

@nickofthyme nickofthyme marked this pull request as ready for review May 16, 2023 14:52
@nickofthyme nickofthyme requested a review from markov00 May 16, 2023 14:52
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 fine to me now, one last small thing: it will be great to have a screenshot for just that particular configuration, so it's easier to understand rather then trying to detect the problem from the small multiple screenshot changes

@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

@nickofthyme nickofthyme merged commit 9711233 into elastic:main May 23, 2023
@nickofthyme nickofthyme deleted the fix-mlta-labels branch May 23, 2023 15:10
nickofthyme pushed a commit that referenced this pull request May 23, 2023
## [57.0.1](v57.0.0...v57.0.1) (2023-05-23)

### Bug Fixes

* **axes:** start of week label on multilayer time axis ([#2035](#2035)) ([9711233](9711233))
* **deps:** update dependency @elastic/eui to ^77.2.0 ([#2032](#2032)) ([93cadcb](93cadcb))
* **deps:** update dependency @elastic/eui to v78 ([#2038](#2038)) ([3feff2c](3feff2c))
* **deps:** update dependency @elastic/eui to v79 ([#2042](#2042)) ([8015830](8015830))
* **deps:** update dependency @elastic/eui to v80 ([#2047](#2047)) ([e6042f3](e6042f3))
* **heatmap:** brushing selection values ([#2028](#2028)) ([6a877b4](6a877b4))
* **heatmap:** wrong axes labels on hover ([#2033](#2033)) ([045fb03](045fb03))
nickofthyme added a commit to elastic/kibana that referenced this pull request Jun 6, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@elastic/charts](https://togithub.com/elastic/elastic-charts) |
[`55.0.0` ->
`57.0.1`](https://renovatebot.com/diffs/npm/@elastic%2fcharts/55.0.0/57.0.1)
|
[![age](https://badges.renovateapi.com/packages/npm/@elastic%2fcharts/57.0.1/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/@elastic%2fcharts/57.0.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/@elastic%2fcharts/57.0.1/compatibility-slim/55.0.0)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/@elastic%2fcharts/57.0.1/confidence-slim/55.0.0)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Bug fixes

- Fixes #148872 relating to multilayer time axes.
- Fixes #158580 relating to heatmap selection with small multiples.

---

### Release Notes

<details>
<summary>elastic/elastic-charts</summary>

###
[`v57.0.1`](https://togithub.com/elastic/elastic-charts/blob/HEAD/CHANGELOG.md#&#8203;5700-httpsgithubcomelasticelastic-chartscomparev5700v5701-2023-04-19)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/v57.0.0...v57.0.1)

##### Bug Fixes

* **axes:** start of week label on multilayer time axis
([#2035](elastic/elastic-charts#2035))
([9711233](elastic/elastic-charts@9711233))
* **deps:** update dependency @elastic/eui to ^77.2.0
([#2032](elastic/elastic-charts#2032))
([93cadcb](elastic/elastic-charts@93cadcb))
* **deps:** update dependency @elastic/eui to v78
([#2038](elastic/elastic-charts#2038))
([3feff2c](elastic/elastic-charts@3feff2c))
* **deps:** update dependency @elastic/eui to v79
([#2042](elastic/elastic-charts#2042))
([8015830](elastic/elastic-charts@8015830))
* **deps:** update dependency @elastic/eui to v80
([#2047](elastic/elastic-charts#2047))
([e6042f3](elastic/elastic-charts@e6042f3))
* **heatmap:** brushing selection values
([#2028](elastic/elastic-charts#2028))
([6a877b4](elastic/elastic-charts@6a877b4))
* **heatmap:** wrong axes labels on hover
([#2033](elastic/elastic-charts#2033))
([045fb03](elastic/elastic-charts@045fb03))

###
[`v57.0.0`](https://togithub.com/elastic/elastic-charts/blob/HEAD/CHANGELOG.md#&#8203;5700-httpsgithubcomelasticelastic-chartscomparev5600v5700-2023-04-19)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/82f67501967a1fad80253889462e3aaac724e060...v57.0.0)

##### Code Refactoring

- enable `noUncheckedIndexedAccess`
([#&#8203;2006](https://togithub.com/elastic/elastic-charts/issues/2006))
([f446cca](https://togithub.com/elastic/elastic-charts/commit/f446cca1691bbe5d7608845b65ea555f74e0f6af))

##### BREAKING CHANGES

-   Enables stricter type option in src and could have
    unexpected changes. This release is meant to serve as a clean break
    in case any issues arise.

###
[`v56.0.1`](https://togithub.com/elastic/elastic-charts/compare/v56.0.0...82f67501967a1fad80253889462e3aaac724e060)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/v56.0.0...82f67501967a1fad80253889462e3aaac724e060)

###
[`v56.0.0`](https://togithub.com/elastic/elastic-charts/blob/HEAD/CHANGELOG.md#&#8203;5600-httpsgithubcomelasticelastic-chartscomparev5500v5600-2023-04-18)

[Compare
Source](https://togithub.com/elastic/elastic-charts/compare/v55.0.0...v56.0.0)

##### Bug Fixes

- **deps:** update dependency
[@&#8203;elastic/eui](https://togithub.com/elastic/eui) to ^76.4.0
([#&#8203;2008](https://togithub.com/elastic/elastic-charts/issues/2008))
([95176e1](https://togithub.com/elastic/elastic-charts/commit/95176e14294b36fce983f53c6c5f278da24f50b1))
- **deps:** update dependency
[@&#8203;elastic/eui](https://togithub.com/elastic/eui) to v77
([#&#8203;2018](https://togithub.com/elastic/elastic-charts/issues/2018))
([c079730](https://togithub.com/elastic/elastic-charts/commit/c079730dd849c34f71608df36938560566ba19d3))
- **interactions:** brushing over origin coordinates
([#&#8203;2013](https://togithub.com/elastic/elastic-charts/issues/2013))
([937feb0](https://togithub.com/elastic/elastic-charts/commit/937feb0fcf1a11c473e45dfb0da36443660e98be))
- **tooltip:** custom tooltip header context
([#&#8203;1989](https://togithub.com/elastic/elastic-charts/issues/1989))
([1e5b861](https://togithub.com/elastic/elastic-charts/commit/1e5b86106ff4c72e5a59b074e0472023ecc68164))

##### Features

- **metric:** trend with string value
([#&#8203;2011](https://togithub.com/elastic/elastic-charts/issues/2011))
([91d7695](https://togithub.com/elastic/elastic-charts/commit/91d76957d88d25e93904f73b845c47d411f4ce32))

##### BREAKING CHANGES

- **tooltip:** The `header` property of `TooltipInfo` type was
simplified to `PointerValue` as to include only relevant properties.
This change is propagated to all other types using `header` as a
`TooltipValue`. The `TooltipInfo.values` used to conditionally pass only
highlighted `TooltipValue`s when using a `customTooltip` and now
*always* passes all `values`.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/elastic/kibana).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS41OC4yIiwidXBkYXRlZEluVmVyIjoiMzUuOTguNCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: nickofthyme <nicholas.partridge@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:axis Axis related issue :timeslip Timeslip related issues :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants