-
Notifications
You must be signed in to change notification settings - Fork 121
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(tooltip): improve positioning with popperjs #651
Conversation
- refactor tooltip logic to account for portal - seperate tooltip and tooltip portal components - update tests to check for new portal condition - set max tooltip width from component js
90a3630
to
f574e6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good changes! I tested it on OS X with Chrome, Safari and FireFox, various chart types, various browser zoom levels via the menu, various pinch zoom levels and various scroll positions (by eg. shrinking the story page to lure out a vertical scrollbar).
323 files changed, many of them apparently unrelated to the goal of the PR, it'd be great to write up those changes so those can be isolated and reviewed too.
Also, a bunch of images changed; it looks initially OK as they seem to be interaction related, but not sure if the image changes are desired, eg. axes and even tooltip got removed here and there, eg.
unless I'm looking at it the wrong way
@monfera yeah I added an eslint rule for react hooks that required some file changes, namely the correct component naming of functional components. This required changing all the The screenshots are off because I was changing the story to allow more uniform padding around the chart. I will add this to the other PR with the other changes to vrt. see 9c534e7 |
Thanks @nickofthyme - makes sense. I suppose it'll cause the tooltip(s) to return to those screengrabs too |
Yup. The first screenshot is now containing the tooltip beyond the chart container, which is an expected change. The placement is set to top and before would fallback to bottom to be contained within the chart element. We now allow placement relative to the window or a defined The second screenshot is also intended. The cursor is now centered relative to the tooltip. There is an option for @markov00 / @rshen91 any thoughts on this? see demo below. Notice the vertical placement of the tooltip relative to the cursor. CurrentNew |
Thanks @nickofthyme I played with it some more around the edge and it's very nice, there's no cropping of the tooltip so it's all good. That it looks cropped is simply due to where the test mouse position is and what area is screenshot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start: LineAlignSetting; | ||
Center: LineAlignSetting; | ||
End: LineAlignSetting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird....I will open a PR to fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, wasn't sure what this was from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I've pushed two small commits to fix some missing tsdocs and exported types
|
||
/** | ||
* {@inheritDoc (Placement:variable)} | ||
* @public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markov00 Is this required? I thought @public
was the default.
/** | ||
* Unit for event (i.e. `time`, `feet`, `count`, etc.). | ||
* Not currently used/implemented | ||
* @alpha | ||
*/ | ||
unit?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
# [19.4.0](v19.3.0...v19.4.0) (2020-05-28) ### Bug Fixes * **partition:** consider legendMaxDepth on legend size ([#654](#654)) ([9429dcf](9429dcf)), closes [#639](#639) ### Features * **partition:** enable grooves in all group layers ([#666](#666)) ([f5b4767](f5b4767)) * **partition:** linked text overflow avoidance ([#670](#670)) ([b6e5911](b6e5911)), closes [#633](#633) * **partition:** monotonic font size scaling ([#681](#681)) ([ea2489b](ea2489b)), closes [#661](#661) * **tooltip:** improve positioning with popperjs ([#651](#651)) ([6512950](6512950)), closes [#596](#596)
🎉 This PR is included in version 19.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [19.4.0](elastic/elastic-charts@v19.3.0...v19.4.0) (2020-05-28) ### Bug Fixes * **partition:** consider legendMaxDepth on legend size ([opensearch-project#654](elastic/elastic-charts#654)) ([20cc6ec](elastic/elastic-charts@20cc6ec)), closes [opensearch-project#639](elastic/elastic-charts#639) ### Features * **partition:** enable grooves in all group layers ([opensearch-project#666](elastic/elastic-charts#666)) ([b1bdfb3](elastic/elastic-charts@b1bdfb3)) * **partition:** linked text overflow avoidance ([opensearch-project#670](elastic/elastic-charts#670)) ([59617db](elastic/elastic-charts@59617db)), closes [opensearch-project#633](elastic/elastic-charts#633) * **partition:** monotonic font size scaling ([opensearch-project#681](elastic/elastic-charts#681)) ([d46767c](elastic/elastic-charts@d46767c)), closes [opensearch-project#661](elastic/elastic-charts#661) * **tooltip:** improve positioning with popperjs ([opensearch-project#651](elastic/elastic-charts#651)) ([61d1d9a](elastic/elastic-charts@61d1d9a)), closes [opensearch-project#596](elastic/elastic-charts#596)
Summary
This replaced our current tooltip positioning logic with popperjs.
Why bother? Well positioning is tricky and based on the current implementation there are some limitations to positioning relative to the window, this comes into play for really small charts.
View on local storybook: http://localhost:9001/?path=/story/bar-chart--test-tooltip-and-rotation
There is now a
Portal
component that is simply a presentation component that renders it's content to the root of the dom. You are able to pass the portal ananchor
in which the portal will use to position itself. This anchor can be anHTMLElement
or apostion
values relative to a givenref
HTMLElement
.These changes also allow greater flexibility in the size, specifically the width, of the tooltip. Currently the
width
is set manually for the tooltip to grow. However, with these changes, the tooltip can have amax-width
and grow to that values without collapsing early. This also applies to anyCustomTooltip
component that is used in place of the default.New Tooltip options
Changes
This works by creating a pseudo anchor element from the calculated
AnchorPosition
values.Why
Popperjs
?Pros:
Cons:
Checklist
Delete any items that are not applicable to this PR.
src/index.ts
(and stories only import from../src
except for test data & storybook)