LG-4526: refactor popover-consuming components #2501
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
🦋 Changeset detectedLatest commit: 0b99dd5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 66 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: -966 B (-0.07%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
stephl3
force-pushed
the
steph/po-refactor-components
branch
4 times, most recently
from
a3766bb
to
21481e5
Compare
stephl3
requested review from
shaneeza,
TheSonOfThomp,
tsck and
bruugey
and removed request for
a team
stephl3
commented
Oct 11, 2024
Comment on lines
+333
to
+335
| export const inlineDefinitionStyles = css` | ||
| white-space: normal; | ||
| `; |
There was a problem hiding this comment.
Added this to ensure text wraps within the tooltip
Comment on lines
-46
to
-49
| /** | ||
| * Number that controls the z-index of the tooltip containing the full label text. | ||
| */ | ||
| popoverZIndex?: number; |
There was a problem hiding this comment.
This was added exclusively for the tooltip for instances of ComboboxChip with long texts: https://github.com/search?q=(org:10gen+OR+org:evergreen-ci+OR+org:mongodb+OR+org:mongodb-js+OR+org:mongodb-labs+OR+org:wiredtiger)+AND+(@leafygreen-ui/chip+OR+%3CChip)+AND+popoverZIndex%3D&type=code
Instead, we can now always use the top layer to ensure proper visual stacking
Comment on lines
-103
to
+86
| {...popoverProps} | ||
| renderMode={RenderMode.TopLayer} |
There was a problem hiding this comment.
From reviewing usage of languageOptions: https://github.com/search?q=(org:10gen+OR+org:evergreen-ci+OR+org:mongodb+OR+org:mongodb-js+OR+org:mongodb-labs+OR+org:wiredtiger)+AND+(@leafygreen-ui/code+OR+%22%3CCode%22)+AND+(languageOptions%3D)&type=code
I observed:
- few enough instances (~8) of using a
Codecomponent w/ aLanguageSwitcher - consistently usage of rendering the
LanguageSwitcheras the highest element
usePortal={false}+ highpopoverZIndexusePortal={true}
- the
LanguageSwitcheris the only open element at any given time
For these reasons, I believe it's safe to always use the top layer. If there's strong reasoning to the contrary, we can surface the renderMode prop for this component, but it doesn't seem necessary for this case
Comment on lines
-186
to
+190
| usePortal={shouldTooltipUsePortal} | ||
| renderMode={RenderMode.TopLayer} |
There was a problem hiding this comment.
(insert github code search link)
| className={tooltipStyles} | ||
| darkMode={darkMode} | ||
| justify={Justify.Middle} | ||
| renderMode={RenderMode.TopLayer} |
There was a problem hiding this comment.
(insert github code search link)
Comment on lines
-85
to
-89
| usePortal = true, | ||
| portalClassName, | ||
| portalContainer, | ||
| portalRef, | ||
| scrollContainer, |
There was a problem hiding this comment.
(insert github code search link)
Comment on lines
-36
to
+39
| usePortal={false} | ||
| align="right" | ||
| justify="middle" | ||
| align={Align.Right} | ||
| justify={Justify.Middle} | ||
| open={typeof hideTooltip === 'boolean' ? !hideTooltip : undefined} | ||
| renderMode={RenderMode.TopLayer} |
There was a problem hiding this comment.
(insert github code search link)
| darkMode={darkMode} | ||
| justify={Justify.Middle} | ||
| renderMode={RenderMode.TopLayer} |
There was a problem hiding this comment.
(insert github code search link)
Comment on lines
+149
to
+159
| timeoutRef.current = setTimeout(() => { | ||
| setOpen(true); | ||
| }, 500); | ||
| }); | ||
| }, 35), | ||
| onMouseLeave: debounce((e: MouseEvent) => { | ||
| userTriggerHandler('onMouseLeave', e); | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| timeoutRef.current = null; | ||
| } |
There was a problem hiding this comment.
moved to this JS approach for delaying the tooltip entry in place of the CSS transition-delay
stephl3
force-pushed
the
steph/po-refactor-components
branch
from
21481e5
to
9852a0d
Compare
stephl3
force-pushed
the
steph/po-refactor-components
branch
from
9852a0d
to
0b99dd5
Compare
stephl3
force-pushed
the
LG-4121-popover-refactor
branch
from
6c8a4cd
to
3cfece1
Compare
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.
✍️ Proposed changes
usePortalprop withrenderModePopover-consuming components🎟 Jira ticket: LG-4526
✅ Checklist
For bug fixes, new features & breaking changes
yarn changesetand documented my changes🧪 How to test changes
Terrence section
Comboboxlive examplerenderModesettingsmultiselect, select long text options, and hover chipsGuideCuelive exampleMultistep DemostoryMenulive examplerenderModesettingsSplitButtonlive examplerenderModesettingsSearchInputlive exampleAdam section
Selectlive examplerenderModesettingsCodelive exampleCopyButtonWith Language Switcherstory and interact withLanguageSwitcherDatePickerlive exampleNumberInputlive exampleUnitSelectUnitSelectButtonPaginationlive exampleWith Current Page OptionsstoryShaneeza section
Tooltiplive examplerenderModesettingsCopyablelive exampleInfoSprinklelive exampleInlineDefinitionlive examplePipelinelive exampleSideNavlive exampleCollapseToggleStepperlive exampleEllipsesStep