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

fix(barSeries): error rendering bars with negative log scale #2407

Merged
merged 21 commits into from
May 7, 2024
Merged

fix(barSeries): error rendering bars with negative log scale #2407

merged 21 commits into from
May 7, 2024

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Apr 11, 2024 •
edited by markov00
Loading

Summary

This PR mainly fixes the rendering of bars on a negative log scale. Also fixes the tooltip for banded bar charts.

Zight Recording 2024-04-11 at 10 27 05 AM

In addition this PR fixes some other issues including

  • Banded bar charts missing lower bound series in tooltip and legend
  • Wrong highlighting of tooltips value and highlighted bar geometries, particularly for banded bar charts.
  • Banded series items in tooltip were being shuffled according to the distance to the cursor position. Now I pre-sort them to not abruptly switch.
  • The Chrome bug from Percentage area charts can have gaps #1053 associated with filling areas containing sharp corners, is now fixed and since using the shared getY1ScaledValueFn and getY0ScaledValueFn functions I just removed this logic to avoid checking.

Details

The bar geometry logic was using custom logic to handle the scaled y1 and y0 values, this was imperfect. This same issue was not present with AreaSeries as we had already addressed it. Using the same logic from area geometries fixed the issue.

Secondly, while testing the above fix for banded BarSeries, I noticed that the tooltip never showed the lower bound item on the tooltip. This again only an issue for bar charts. The issue is that unlike the AreaSeries the BarSeries geometry is a single geometry for the upper and lower whereas the area is 2 points. So simply adding the y0 geometry to the indexedGeometryMap solved the issue, see bed2b45.

Issues

fix #2406, fix #2408, fix #607

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)
  • 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

@nickofthyme nickofthyme added :data Data/series/scales related issue :tooltip Related to hover tooltip :xy Bar/Line/Area chart related labels Apr 11, 2024
@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

elastic-datavis bot and others added 9 commits April 11, 2024 17:59
@elastic-datavis
test(vrt): update screenshots [skip ci]
464b807
@nickofthyme
Merge branch 'main' into fix-neg-log
b886207
@nickofthyme
fix: render highlighter and geom matcher
6d3e0f3 
- remove upsidedown height logic, use negative height and handle render accordingly.
- add range helper utils to check for values in first and last half of given range.
@nickofthyme
fix: minBarHeight issue
d51ac12 
- This comes from the change to use the shared `getY1ScaledValueFn` that adds a little extra for elastic#1053.
- The chrome bug causing elastic#1053 has now been fixed, so removing this code.
@nickofthyme
fix: pre-sort tooltip values to prevent swapping when cursor moves
b3b1d36
@nickofthyme
Merge remote-tracking branch 'origin/fix-neg-log' into fix-neg-log
4381347
@nickofthyme
chore: remove old updated screenshots
ddd3d99
@nickofthyme
fix: y0 on bars where min height it applied
bf89632 
- also fixes min height for banded bars that cross `0`
@nickofthyme
Merge branch 'main' into fix-neg-log
a26581f
nickofthyme
nickofthyme commented Apr 26, 2024
View reviewed changes
Copy link
Collaborator Author

@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.

Just leaving comments to explain specific code changes.

packages/charts/src/chart_types/xy_chart/rendering/bars.ts Outdated Show resolved Hide resolved
packages/charts/src/chart_types/xy_chart/rendering/utils.ts Outdated Show resolved Hide resolved
...ages/charts/src/chart_types/xy_chart/state/selectors/get_tooltip_values_highlighted_geoms.ts Show resolved Hide resolved
packages/charts/src/utils/common.tsx Outdated Show resolved Hide resolved
packages/charts/src/utils/geometry.ts Outdated Show resolved Hide resolved
packages/charts/src/chart_types/xy_chart/rendering/bars.ts Outdated Show resolved Hide resolved
packages/charts/src/chart_types/xy_chart/rendering/bars.ts Outdated Show resolved Hide resolved
packages/charts/src/chart_types/xy_chart/rendering/bars.ts Outdated Show resolved Hide resolved
packages/charts/src/chart_types/xy_chart/renderer/dom/highlighter.tsx Outdated Show resolved Hide resolved
@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

@nickofthyme nickofthyme marked this pull request as ready for review April 30, 2024 21:56
@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

@nickofthyme nickofthyme enabled auto-merge (squash) May 7, 2024 02:16
@nickofthyme nickofthyme disabled auto-merge May 7, 2024 05:04
@nickofthyme nickofthyme merged commit 4ab6d8f into elastic:main May 7, 2024
14 checks passed
@nickofthyme nickofthyme deleted the fix-neg-log branch May 7, 2024 05:05
nickofthyme pushed a commit that referenced this pull request May 20, 2024
@semantic-release-bot
chore(release): 65.0.0 [skip ci]
4f266ca 
# [65.0.0](v64.1.0...v65.0.0) (2024-05-20)

### Bug Fixes

* **barSeries:** error rendering bars with negative log scale ([#2407](#2407)) ([4ab6d8f](4ab6d8f))
* **deps:** update dependency @elastic/eui to ^93.5.1 ([#2375](#2375)) ([35ed956](35ed956))
* **deps:** update dependency @elastic/eui to ^93.5.2 ([#2386](#2386)) ([e26c6dd](e26c6dd))
* **deps:** update dependency @elastic/eui to ^93.6.0 ([#2393](#2393)) ([40f2b7b](40f2b7b))
* **deps:** update dependency @elastic/eui to ^94.3.0 ([#2424](#2424)) ([cff5181](cff5181))
* **deps:** update dependency @elastic/eui to v94 ([#2409](#2409)) ([67c814f](67c814f))
* **deps:** update dependency @playwright/test to ^1.43.0 ([#2388](#2388)) ([42f86d7](42f86d7))
* **deps:** update dependency @playwright/test to ^1.43.1 ([#2413](#2413)) ([79b1c7f](79b1c7f))
* **deps:** update dependency json-schema-to-typescript to v14 ([#2414](#2414)) ([785f635](785f635))
* **deps:** update dependency json-schema-to-typescript to v14.0.4 ([#2421](#2421)) ([790170a](790170a))
* **legend:** custom legend covered by background ([#2366](#2366)) ([5b9ffac](5b9ffac))

### Features

* add support for start day of week on MLT axis ([#2362](#2362)) ([3aac1f0](3aac1f0))
* **Legend:** change click on item behaviour ([#2427](#2427)) ([b1c72df](b1c72df))
* **legend:** change click on item behaviour ([#2431](#2431)) ([b03bdd0](b03bdd0))
* **legend:** Improve interactions legend labels ([#2418](#2418)) ([384baac](384baac))
* **legend:** select legend statistic value ([#2355](#2355)) ([a602838](a602838))
* **metric:** support array of values ([#2428](#2428)) ([e448bd7](e448bd7))

### Reverts

* **legend:** change click on item behaviour ([#2429](#2429)) ([cc438a1](cc438a1)), closes [#2427](#2427)

### BREAKING CHANGES

* **legend:** The legend modifier key has been changed to CTRL (or CMD on Mac) from SHIFT. The SHIFT key will no longer have any effect on click.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 Awaiting requested review from markov00

Assignees
No one assigned
Labels
:data Data/series/scales related issue :tooltip Related to hover tooltip :xy Bar/Line/Area chart related
Projects
None yet
Milestone
No milestone
2 participants
@nickofthyme @markov00