-
Notifications
You must be signed in to change notification settings - Fork 63
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
LG-4525: consolidate-usePortal-into-renderMode code mod #2484
base: LG-4121-popover-refactor
Are you sure you want to change the base?
Conversation
* OverlayContext, useOverlayContext, and OverlayProvider * useOverlay to register, remove, and track isTopMostOverlay * Export overlay context related components and hooks and include provider in LG provider * README and changeset * Fix type and lint * Update registerOverlay logic and use more explicit generic type names
* PopoverContext supports additional props and marks isPopoverOpen and setIsPopoverOpen deprecated * Replace PortalContext with newly extended PopoverContext * Changesets * Add todo in select spec and update todos with jira tix
…ClassName prop (#2457) * Refactor popover styles * Remove redundant contentClassName prop * Clean up Popover positioning logic * Changesets * Reorg code * useIsomorphicLayoutEffect instead of useMemo * Update changesets * Revert changes
* Add @floating-ui/react dependency * Clean up Popover stories * Replace position utils and usePopoverPositioning with useFloating * Remove replaced positioning hook and utils * Fix adjustOnMutation and stories * Changeset * Add useMergeRefs hook * Address feedback
* Update Popover default spacing from 10 to 4 * Changeset * Update README and stories
…2474) * Remove justify fit from @leafygreen-ui/popover * Remove justify fit usage in packages using Popover * Changeset
🦋 Changeset detectedLatest commit: 54664af The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
7e740ac
to
c133311
Compare
Size Change: 0 B Total Size: 1.36 MB ℹ️ View Unchanged
|
tools/codemods/src/codemods/consolidate-popover-usePortal-renderMode-props/transform.ts
Outdated
Show resolved
Hide resolved
const isPropValueString = typeof propValue === 'string'; | ||
const attrValueNode = isPropValueString | ||
? j.stringLiteral(propValue) | ||
: j.jsxExpressionContainer(j.literal(propValue)); |
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.
It's been a while, so I don't remember; what does jsxExpressionContainer
return?
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.
it wraps the value in curly brackets
}; | ||
|
||
/** | ||
* Example transformer function to consolidate props |
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.
Can you update this line, explaining what this codemod does?
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 looks great! A few comments I have:
- Can you update the README to include this codemod? We should also remove the current codemods in the README because those currently are not consumer-facing.
- Should we also add a section in
Popover
's README about this codemod? - You mentioned before about creating codemods for components that consume
Popover
such asSelect
,Tooltip
, etc. What's the plan for those components? We could technically add all those components into this codemode. We can create an array of components that need updating and loop through them in the transformer file. - Were you able to test this locally with
yarn lg codemod consolidate-usePortal-into-renderMode <path>
?
done!
I have some README changes in PR #2482 that I'd want to build off of. I will revisit this with work in this ticket
great, I updated the code mod to cover those components as well
I tested it locally on the LGUI repo, and it's able to correctly modify most instances. It errors on a few unique cases which I might expect it to but curious what degree of effectiveness we should expect / be aiming for:
|
6bd427a
to
54664af
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.
LGTM!
componentNames = [ | ||
'Code', | ||
'Combobox', | ||
'DatePicker', | ||
'InfoSprinkle', | ||
'InlineDefinition', | ||
'Menu', | ||
'NumberInput', | ||
'Popover', | ||
'SearchInput', | ||
'Select', | ||
'SplitButton', | ||
'Tooltip', | ||
], |
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.
Check these
- Combobox
- DatePicker (DatePickerMenu)
- GuideCue (Beacon)
- Menu
- SplitButton
- SearchInput
- Select
- Code (LanguageSwitcher)
- DatePicker (MM/YY selectors)
- NumberInput (UnitSelect)
- Pagination
- Tooltip
- Code (CopyButton)
- Copyable
- GuideCue (TooltipContent)
- InfoSprinkle
- InlineDefinition
- NumberInput (UnitSelectButton)
- Pipeline
- SideNav
- Stepper
Add default and consolidate for the following:
- Combobox
- Menu
- Select
- SplitButton
- Tooltip
Remove props and always use top layer for the following:
- Code
- DatePicker
- GuideCue
- InfoSprinkle
- InlineDefinition
- NumberInput
- SearchInput
Remove shouldTooltipUsePortal
and always use top layer:
- Copyable
Does not need mod:
- Pagination
- Pipeline
- SideNav
- Stepper
6c8a4cd
to
3cfece1
Compare
✍️ Proposed changes
consolidate-usePortal-into-renderMode
code mod🎟 Jira ticket: LG-4525
✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changes