-
Notifications
You must be signed in to change notification settings - Fork 403
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
Tooltips using React DOM #364
Tooltips using React DOM #364
Conversation
Oh and two other things, I'll squash out my first commit here on merge as it was my original implementation attempt, and this would be a good follow-up to test after merging the testing snapshot PR #353. |
Codecov Report
@@ Coverage Diff @@
## master #364 +/- ##
==========================================
- Coverage 36.46% 36.08% -0.39%
==========================================
Files 123 124 +1
Lines 5953 6038 +85
Branches 1238 1262 +24
==========================================
+ Hits 2171 2179 +8
- Misses 3240 3303 +63
- Partials 542 556 +14
Continue to review full report at Codecov.
|
One quick comment about I'm especially worried about the accessibility of putting a DOM element somewhere not related. |
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.
I really like the behavior ! It feels really great ! There is one caveat: there is no way to copy the text that's inside the tooltip because it's disappearing too quick. This is not a regression because we can't do it in current master either, so we can discuss this later. I don't have a good idea to allow it anyway.
My main comment is to use position: fixed
to position the tooltip. But otherwise I like it out of the app in a separate DOM element (like you did).
Another concern is how you use the state in the Tooltip component.
otherwise: nits, comments, and something about accessibility.
res/style.css
Outdated
/* Create a stacking context so that tooltips can be placed over the root element. */ | ||
position: relative; | ||
z-index: 0; | ||
} |
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.
I think you don't need this if you use position: fixed
for the tooltip element.
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.
I'm not convinced fixed
would be a better implementation for this behavior. I like the explicitness of creating a stacking context and controlling the structure of the parent containers. I'm a little scared of the implicit stacking behavior of fixed position things, mainly because I can't find great documentation on it, especially since the behavior changed ~2012 from looking into it.
this._isMounted = true; | ||
// Create a DOM node outside of the normal heirarchy. | ||
const el = document.createElement('div'); | ||
el.className = 'tooltipMount'; |
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.
how about always keeping the mount element in index.html ? This could also make it easier to debug with the inspector (being able to expand it :) )
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.
Having it dynamic means we don't have to worry about treading on someone else's Tooltip. I don't think practically that would happen, but I was still concerned about a collision with 2 tooltip instances.
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.
Also I was debugging the Tooltips by removing the ternary in the parent element so that the tooltip would always show.
src/content/components/Tooltip.js
Outdated
} | ||
|
||
render() { | ||
return <div/>; |
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.
you can just return null
.
/** | ||
* This is really ugly, but the tooltip needs to be outside of the normal | ||
* DOM heirarchy so it isn't clipped by some arbitrary stacking context. | ||
*/ |
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.
not true if we use position: fixed
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.
I tried it locally, and I was still running into stacking issues. fixed
would break out of the overflow, but would still obey some stacking context rules.
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.
to make my mind clearer: I think it's better anyway to have the tooltip element outside of the app's DOM hierarchy, but that using the fixed positioning would make it easier to position.
but this is not really a big deal
src/content/components/Tooltip.js
Outdated
ReactDOM.render( | ||
<div className='tooltip' style={style} ref={this._getMountElement}> | ||
{children} | ||
</div>, |
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.
nit: indentation
|
||
.flameChartCanvasTooltip { | ||
padding: 5px; | ||
} |
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.
just wondering why it's different for this tooltip ? I couldn't really debug and find out, because of the disappearing tooltip :)
Please add a comment explaining why, but I'd rather have something that works for all tooltips.
overflow: hidden; | ||
text-overflow: ellipsis; | ||
transition: opacity 250ms; | ||
box-sizing: border-box; |
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.
nit: please put box-sizing
closer to max-width
as they're related.
bonus points for a new line after display
src/content/components/Tooltip.css
Outdated
.tooltipLabel { | ||
display: block; | ||
width: 60px; | ||
color: var(--theme-highlight-gray); |
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.
this is way too pale for accessibility. See https://snook.ca/technical/colour_contrast/colour.html#fg=B4BABF,bg=FFFFFF
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.
Still too pale :)
You could use #7B747A
for example.
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.
still this
{category.name} | ||
</div> | ||
{resourceOrFileName} | ||
</div> |
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.
I think I'd rather see this structure handled (and enforced) by the Tooltip
component, especially if we're using the CSS classes that the Tooltip component defines.
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.
Hmm... I'm not really sure how to handle that. I'd like the Tooltip to only handle the tooltip logic. I'd like the presentation to be free-form and up to the controlling element. When I'm implementing something per-panel I'd prefer to do what I want. Placing some shared style in the Tooltip css seemed the most convenient place.
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.
Yeah, I think we can wait until we use tooltips at other places and see what we can generalize rather than decide now.
src/content/components/Tooltip.css
Outdated
top: 0; | ||
left: 0; | ||
width: 100%; | ||
height: 100%; |
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.
most of it should be useless when using position: fixed for the tooltip.
position:fixed stops doing what you want as soon as there's a transform on an ancestor. Elements with transforms are containing blocks for position:fixed descendants. |
17c01cf
to
a220158
Compare
src/content/components/Tooltip.css
Outdated
@@ -67,3 +68,46 @@ | |||
width: 60px; | |||
color: var(--theme-highlight-gray); | |||
} | |||
|
|||
.tooltipTiming { |
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.
I may have rewritten my history incorrectly here. I accidentally squashed my changes on the previous commit.
src/content/components/Tooltip.js
Outdated
@@ -22,22 +22,22 @@ export default class Tooltip extends PureComponent { | |||
|
|||
state: { | |||
interiorElement: HTMLElement | null, | |||
interiorElementRAF: HTMLElement | null, | |||
isNewContentLaidOut: boolean, |
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.
I refactored this a bit, it still has the same logic as before, but with hopefully better words and comments.
Oh, I didn't know this. But anyway I'm still in favor of having the Tooltip DOM element quite at the top of the DOM tree, so this shouldn't get in the way. I just think this is simpler and makes more sense to use |
You'll need to rebase, there is a conflict with latest master. |
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.
some more comments, most important to me is the accessibility for one of the color used.
I'd really like to make it work with position: fixed
but it's not worth spending too much time on this either.
title={title}/> | ||
onMouseOut={this._onMouseOut}/> | ||
{ | ||
tooltipContents |
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.
we don't check if we're currently "selecting" here (like isDragging
below). I think you can easily get this information with the boolean state profileView.selection.isModifying
.
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.
I'm fine doing it in a separate patch, please file an issue if you'd prefer this :)
src/content/components/Tooltip.css
Outdated
.tooltipLabel { | ||
display: block; | ||
width: 60px; | ||
color: var(--theme-highlight-gray); |
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.
Still too pale :)
You could use #7B747A
for example.
a220158
to
287d0de
Compare
The tooltips are position fixed now, and I've rebased. |
youhou, thanks ! |
Ah, I didn't see your other comments. |
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.
I still don't really like the logic where we need to render twice -- which means 2 reflows but I don't have a better idea. Maybe we'll come back at this later.
Please fix the color for accessibility, and the last nit. Also please file a bug so that the offset takes the current dpi into account.
componentWillReceiveProps(nextProps: Props) { | ||
if (nextProps.children !== this.props.children) { | ||
// If the children are different, allow them to do an initial lay out on the DOM. | ||
this.setState({isNewContentLaidOut: false }); |
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.
nit: space after {
src/content/components/Tooltip.css
Outdated
.tooltipLabel { | ||
display: block; | ||
width: 60px; | ||
color: var(--theme-highlight-gray); |
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.
still this
This tooltip is a little weird, as it uses ReactDOM's render method to insert a DOM component outside of the normal DOM position. This is needed because tooltips need to be outside of the stacking context of the parent elements.
a2eb0d2
to
86294f6
Compare
This adds a styled
Tooltip
component. Tooltips are surprisingly difficult, because aTooltip
component wants to be controlled and instantiated from some arbitrary point in the DOM and component hierarchy. However, tooltips DOM elements need to be created outside of that point to avoid clipping and placement issues with the current stacking context. React does not have a good built-in metaphor to do this.Approaches
React DOM (Currently implemented)
With this implementation it's nice because each
Tooltip
component can be local to where it is created, but it doesn't create any DOM nodes until it's actually used. The React metaphor is maintained when actually usingTooltip
component, as tooltips can be created locally by individual components. However the actual implementation has some funkiness where it re-renders manually on props changes using React DOM. I don't foresee any way this is brittle at the moment, as it works similarly to how any component manually editing the DOM would work, but uses the React DOM render method.Using redux:
I evaluated placing the tooltip into the Redux store, but I was afraid of the performance impacts of constantly updating tooltip positions on
mousemove
.Shared react component
This implementation would create a separate
Tooltip
mounted at startup, and existing for the lifetime of the application. It would then be passed somehow (either manually through props, globally, or through a connected component) and those components could set and unset the text.React Context
Another approach could be to use a React context, but the docs make it sound like we can't guarantee on this feature continuing to exist. The pros would be a pretty clear abstraction for how the Tooltip works, but then the cons is trusting on a React API with an unclear future.
In conclusion
I'm fine with this current ReactDOM approach, but would listen to feedback if anyone feels strongly against it.