-
Notifications
You must be signed in to change notification settings - Fork 119
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
fix(RC-25445): use querySelectorPortal in pcln-menu and pcln-popover only when provided as a prop #1510
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1510 +/- ##
=======================================
Coverage 93.75% 93.75%
=======================================
Files 149 149
Lines 11275 11275
Branches 690 685 -5
=======================================
Hits 10571 10571
Misses 685 685
Partials 19 19 ☔ View full report in Codecov by Sentry. |
packages/menu/src/Menu/Menu.jsx
Outdated
@@ -69,14 +69,14 @@ function Menu({ | |||
trapFocus={trapFocus} | |||
width={width} | |||
zIndex={zIndex} | |||
querySelectorPortal={`.${querySelectorPortal}`} | |||
querySelectorPortal={querySelectorPortal && `.${querySelectorPortal}`} |
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 would coerce to false
. i think we want undefined (or construct the props and omit this if we don't want it). in that regard, it's not unlike Authorization header conversations.
querySelectorPortal ? `.${querySelectorPortal}` : undefined
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.
@sdalonzo thanks for flagging! updated
I noticed a hydration issue with
CurrencySelector
in@pcln/selectors
while trying to migrate alt-landing app (which is only used for Airport Rental Cars landing page) to use DS6+React 18.@pcln/selectors > CurrencySelector
is used in@pcln/white-label-components
>Header
.@sdalonzo pointed out there was a bug with pcln-menu and pcln-popover (thank you!). In this PR I am making updates and adding test cases so that
querySelectorPortal
is only used inpcln-menu
andpcln-popover
only when provided as a prop.