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(heatmap): brushing selection values #2028

Merged
merged 4 commits into from
May 22, 2023
Merged

fix(heatmap): brushing selection values #2028

merged 4 commits into from
May 22, 2023

Conversation

nickofthyme
Copy link
Collaborator

Summary

Fixes a bug related to heatmap brushed and click selection where brushing a given panel would brush into the adjacent panel.

Before

Screen Recording 2023-04-21 at 12 04 30 PM

After

Screen Recording 2023-04-21 at 12 03 31 PM

Details

The brushing logic to pick the correct cell range was not correctly normalizing the pointer coordinates with respect to the projected chart area. Accounting for the axes dimensions to the left of the chart correctly normalizes the coordinate to be used in the scaled value lookup.

Issues

Fixes #2024

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)

@nickofthyme nickofthyme added :heatmap Heatmap/Swimlane chart related issue :small multiples Small multiples/trellising related issues labels Apr 21, 2023
markov00
markov00 reviewed Apr 26, 2023
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have to admit that this looks like a tech debt (not your changes, but the overall coordinate to panel coordinates). Is not clear when the coordinates are relative to a panel or to the overall area. Do you mind checking the code again and see if we can specify that better?

@nickofthyme
Copy link
Collaborator Author

I have to admit that this looks like a tech debt (not your changes, but the overall coordinate to panel coordinates). Is not clear when the coordinates are relative to a panel or to the overall area. Do you mind checking the code again and see if we can specify that better?

Yeah I agree, the whole panel coordinate logic can be simplified and shared across all chart types. For now I clarified the which values are in the panel coordinate system. See ca1b117

@nickofthyme nickofthyme requested a review from markov00 May 5, 2023 18:55
@nickofthyme nickofthyme enabled auto-merge (squash) May 5, 2023 18:55
@nickofthyme
Copy link
Collaborator Author

@markov00 Are you okay to merge this?

@nickofthyme
Copy link
Collaborator Author

@elasticmachine merge upstream

markov00
markov00 approved these changes May 22, 2023
View reviewed changes
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.

Tested again, thanks for the changes to clarify the variables.
It looks good to me!

@nickofthyme nickofthyme merged commit 6a877b4 into elastic:main May 22, 2023
@nickofthyme nickofthyme deleted the fix-heatmap-brushing branch May 22, 2023 15:03
nickofthyme pushed a commit that referenced this pull request May 23, 2023
@semantic-release-bot
chore(release): 57.0.1 [skip ci]
49defe1 
## [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 nickofthyme mentioned this pull request May 26, 2023
Merged
1 task
nickofthyme added a commit to elastic/kibana that referenced this pull request Jun 6, 2023
@renovate @nickofthyme
@kibanamachine @stratoula
Update dependency @elastic/charts to v57.0.1 (main) (#155749)
be24483 
[![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
Reviewers

@markov00 markov00 markov00 approved these changes

Assignees
No one assigned
Labels
:heatmap Heatmap/Swimlane chart related issue :small multiples Small multiples/trellising related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug brushing Heatmap with small multiples
3 participants
@nickofthyme @markov00 @elasticmachine