fix(popover): replace displayName checks#21550
fix(popover): replace displayName checks#21550tay1orjones merged 3 commits intocarbon-design-system:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21550 +/- ##
==========================================
- Coverage 94.39% 94.39% -0.01%
==========================================
Files 536 536
Lines 43679 43675 -4
Branches 6278 6266 -12
==========================================
- Hits 41232 41228 -4
Misses 2308 2308
Partials 139 139
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // TODO: Stop relying on `displayName` checks by moving `Toggletip` | ||
| // subcomponents into their own files that avoid the `Popover` <-> | ||
| // `Toggletip` circular dependency. Then replace these `displayName` checks | ||
| // with `isComponentElement(item, ...)` checks. | ||
| const isToggletipButton = item?.type?.displayName === 'ToggletipButton'; | ||
| const isToggletipContent = item?.type?.displayName === 'ToggletipContent'; | ||
| const isPopoverContent = isComponentElement(item, PopoverContent); |
There was a problem hiding this comment.
Harkening back to this conversation, #18971 (comment), I just want to call out that removing the implicit displayName duck typing could theoretically break some setups. Not a blocker to merge though.
Also, these checks intentionally relied on the displayName duck typing to fix this issue, #17225. I wrote a test for it though. So, if that's passing, and the Toggletip autoAlign stories look good, then I think all the bases are covered?
There was a problem hiding this comment.
I think all the bases are covered?
I’m not sure whether that question was directed at me, but if it was, I agree. Hopefully, we can remove all displayName based functionality from the codebase by the time v12 is released.
| (enableFloatingStyles && | ||
| item?.type['displayName'] === 'ToggletipButton') | ||
| ) { | ||
| if (enableFloatingStyles && !isPopoverContent) { |
There was a problem hiding this comment.
It does appear like part of the original conditional is lost here. It originally fell into this block also when the item is a ToggletipButton.
There was a problem hiding this comment.
I don't think this code change resulted in a behavioral difference.
The previous condition was:
(enableFloatingStyles && item?.type !== PopoverContent) ||
(enableFloatingStyles && item?.type['displayName'] === 'ToggletipButton')Because the two checks were combined using the logical OR operator, the ToggletipButton clause would only have affected behavior if the first condition evaluated to false for ToggletipButton. However, that is not the case. ToggletipButton already satisfies item?.type !== PopoverContent, so it entered the block previously. It continues to do so under the updated condition, enableFloatingStyles && !isPopoverContent.
In other words, this change simplifies the logic by removing a redundant clause without changing its behavior.
If you are seeing a regression in a specific story or test, could you share the details so I can take a look?
|
I'm not sure what the next step should be here. |
tay1orjones
left a comment
tay1orjones
left a comment
There was a problem hiding this comment.
Sorry for the delay, this looks good to me.
sangeethababu9223
added
status: ready to merge 🎉
and removed
status: one more review 👀
labels
0f0fbbe
3 participants
No issue.
Replaced
displayNamechecks inPopover.Changelog
Changed
displayNamechecks inPopover.Testing / Reviewing
Run tests.
PR Checklist
As the author of this PR, before marking ready for review, confirm you:
Updated documentation and storybook examplesMore details can be found in the pull request guide