-
-
Notifications
You must be signed in to change notification settings - Fork 533
fix: remove tooltip rendered element from DOM when is not showing #932
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
Merged
+228
−150
Conversation
This file contains hidden or 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
danielbarion
commented
Feb 1, 2023
Test snapshots not updated yet |
Tests will need to be fixed, since they assumed the tooltip was always in the DOM. After that, this should be good to merge. |
gabrieljablonski
approved these changes
Feb 9, 2023
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.
The tests are simple enough that we probably should be fine for now without them.
oshi97
added a commit
to yext/studio
that referenced
this pull request
Jul 20, 2023
This PR updates the preview to be passed into an iframe via react portal. This makes things like fixed styling work as expected within the iframe. I also added an e.preventDefault() which will prevent previewed pinks from being navigable. This is preferable over turning off pointer-events completely so that styling still appears as expected. react-tooltip does not work out of the box inside an iframe, so we have to manually tell it the correct coordinates to render at. It's also impossible for an iframe to render overflow content, so to render the tooltip over the preview like we do in one of our screenshots, I shifted the contents of the iframe down by 50px to make extra space for the tooltip. https://github.com/yext/studio-prototype/blob/e5318a3f941543db21d101bb474e29fe675a166f/e2e-tests/__screenshots__/error-component-preview.spec.ts/components-with-parsing-errors-2.png In order to do this I had to update react-tooltip to get access to new props. However, in 5.7.5 react-tooltip made it so that when the tooltip is not open, it is no longer rendered to the DOM, as opposed to having visibility hidden. This broke our unit tests so I had to update those. ReactTooltip/react-tooltip#932 J=SLAP-2821 TEST=manual I can click on a component within the preview (which is now in an iframe) and the active component will be updated component highlighting still works as expected.
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.
first show
when the element is not rendered (the position was miscalculated)closes #918