fix(components): tooltips #2413
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 similar comments
jare-talend
suggested changes
Sep 18, 2019
| }), | ||
| )} | ||
|
|
||
| {visible && | ||
| ReactDOM.createPortal( | ||
| <div className={theme['tc-tooltip-container']} style={style}> | ||
| <div className={theme['tc-tooltip-container']} style={getTooltipPosition().style}> |
There was a problem hiding this comment.
we can use placement, style declared above
output/components.eslint.txt
Outdated
| @@ -42,13 +42,21 @@ The react/require-extension rule is deprecated. Please use the import/extensions | |||
| /home/travis/build/Talend/ui/packages/components/src/SubHeaderBar/SubHeaderBar.test.js | |||
| 4:8 error 'Container' is defined but never used no-unused-vars | |||
|
|
|||
| /home/travis/build/Talend/ui/packages/components/src/TooltipTrigger/TooltipTrigger.component.js | |||
| 193:10 error 'placement' is assigned a value but never used no-unused-vars | |||
There was a problem hiding this comment.
please fix the proptypes definition and remove them if not used
vbocquet
suggested changes
Sep 18, 2019
packages/components/src/TooltipTrigger/TooltipTrigger.component.js
Outdated
Show resolved
Hide resolved
packages/components/src/TooltipTrigger/TooltipTrigger.component.js
Outdated
Show resolved
Hide resolved
packages/components/src/TooltipTrigger/TooltipTrigger.component.js
Outdated
Show resolved
Hide resolved
1 similar comment
nicomaligne
reviewed
Sep 18, 2019
| let dimensions; | ||
| try { | ||
| // eslint-disable-next-line react/no-find-dom-node | ||
| dimensions = ReactDOM.findDOMNode(refContainer.current).getBoundingClientRect(); |
There was a problem hiding this comment.
Just a question about this change, I don't understand why you need to find your ref in the domnode, since the ref is normally a pointer to the domnode ?
There was a problem hiding this comment.
Because actually ref is not always referencing a DOM node, but just a pointer like you said, so there is no way to access to its getBoudingClientRectmethod.
…ps' into frassinier/fix/components/tooltips
1 similar comment
1 similar comment
jare-talend
approved these changes
Sep 18, 2019
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the problem this PR is trying to solve?
Tooltips can be shown twice at the same time
What is the chosen solution to this problem?
Use
React.Fragmentand better timeout managementPlease check if the PR fulfills these requirements
[ ] This PR introduces a breaking change