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): limit brush tool #1270

Merged
merged 29 commits into from Aug 20, 2021
Merged

fix(heatmap): limit brush tool #1270

merged 29 commits into from Aug 20, 2021

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Jul 27, 2021 •
edited

Summary

This PR restricts the brush tool to the chart container so it doesn't overflow.

Before

Jul-28-2021 broke before

After

Aug-19-2021.08-33-31.mp4

Details

Issues

Fixes #1216

Checklist

  • The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
  • Whenever possible, please check if the closing issue is connected to a running GH project
  • Unit tests were updated or added to match the most common scenarios

@rshen91 rshen91 added the :heatmap Heatmap/Swimlane chart related issue label Jul 27, 2021
@rshen91 rshen91 changed the title feat(heatmap): limit brush tool through brush area fix(heatmap): limit brush tool through brush area Jul 27, 2021
@rshen91 rshen91 changed the title fix(heatmap): limit brush tool through brush area fix(heatmap): limit brush tool overflow Jul 27, 2021
@rshen91 rshen91 changed the title fix(heatmap): limit brush tool overflow fix(heatmap): limit brush tool container Jul 27, 2021
@rshen91 rshen91 marked this pull request as ready for review July 27, 2021 19:55
@rshen91 rshen91 changed the title fix(heatmap): limit brush tool container fix(heatmap): limit brush tool Jul 27, 2021
markov00
markov00 reviewed Jul 28, 2021
View reviewed changes
packages/charts/src/chart_types/heatmap/state/selectors/get_brush_area.ts Outdated
top: start.y,
left: start.x,
width: end.x - start.x - chartDimensions.left,
height: reachXAxis ? dragShape!.height - start.y : end.y - start.y,
Copy link
Member

Choose a reason for hiding this comment

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

dragShape can be undefined, we should be careful here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, good catch. I added checking for dragShape in 5fc5f9c

@rshen91 rshen91 marked this pull request as draft July 28, 2021 13:04
@rshen91 rshen91 marked this pull request as ready for review July 28, 2021 15:50
@rshen91 rshen91 requested a review from markov00 July 28, 2021 15:53
@rshen91
Copy link
Contributor Author

rshen91 commented Jul 28, 2021

jenkins test this

nickofthyme
nickofthyme approved these changes Jul 29, 2021
View reviewed changes
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 LTGM.

@rshen91
Copy link
Contributor Author

rshen91 commented Jul 29, 2021

jenkins test this

@markov00 markov00 added :interactions Interactions related issue bug Something isn't working and removed Impact:Medium labels Jul 30, 2021
@rshen91
Copy link
Contributor Author

rshen91 commented Aug 2, 2021

jenkins test this

@rshen91
Copy link
Contributor Author

rshen91 commented Aug 16, 2021

jenkins, test this

@monfera
Copy link
Contributor

monfera commented Aug 18, 2021

Do we want to continue to sport the unusual white border around the actually selected, snapped area? I get it if it needs to be a separate PR but is there consensus toward solving it?

monfera
monfera reviewed Aug 18, 2021
View reviewed changes
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

The code changes look good!

I see a bit of grey flashing on the very bottom of the "After" version gif, toward the end of the gif recording, not sure if it means that the issue scope is not fully met, so I'm planning to approve right after the acceptability of that flashing is clarified

Not the scope of this PR, I can't vouch for the preexisting grey/white/grey banding so I'd leave it to @markov00 to consider the timing of a design update there

@rshen91
Copy link
Contributor Author

rshen91 commented Aug 19, 2021

Good call @monfera,

no rush to approve this PR or anything, but the refactoring done since those initial screenshots seem to have fixed the issue. I'll update the after screenshot in the issue too. Here is a new giphy:

Aug-19-2021.08-33-31.mp4

@rshen91 rshen91 requested a review from monfera August 19, 2021 14:38
markov00
markov00 approved these changes Aug 20, 2021
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.

It works correctly after the last changes.
Please merge master and update the screenshots accordingly
Good to merge after that

monfera
monfera approved these changes Aug 20, 2021
View reviewed changes
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

LGTM!

I's not the making or the topic of this PR, could you please create and link an issue to this PR to discuss the grey/white/grey nested selection box styling concern? I think the simplest solution would be the removal of the inner grey rectangle, which looks off, is an unusual aesthetic. But maybe there have been discussions about it

@rshen91 rshen91 merged commit 509cf42 into elastic:master Aug 20, 2021
nickofthyme pushed a commit that referenced this pull request Aug 23, 2021
@semantic-release-bot
chore(release): 34.2.0 [skip ci]
a7c19ce 
# [34.2.0](v34.1.1...v34.2.0) (2021-08-23)

### Bug Fixes

* **heatmap:** limit brush tool ([#1270](#1270)) ([509cf42](509cf42)), closes [#1216](#1216)

### Features

* **goal:** dark mode with theme controls ([#1299](#1299)) ([3a583e5](3a583e5))
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 34.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Aug 23, 2021
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

@monfera monfera monfera approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

Assignees
No one assigned
Labels
bug Something isn't working :heatmap Heatmap/Swimlane chart related issue :interactions Interactions related issue released Issue released publicly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The brushTool renders outside the projection area
4 participants
@rshen91 @monfera @nickofthyme @markov00