-
Notifications
You must be signed in to change notification settings - Fork 842
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
Pass EuiSuperSelect's popoverClassName to the popover's panel #2068
Pass EuiSuperSelect's popoverClassName to the popover's panel #2068
Conversation
c9d2a57
to
7e0748d
Compare
@@ -176,6 +176,10 @@ export class EuiSuperSelect extends Component { | |||
popoverClassName | |||
); | |||
|
|||
const popoverPanelClasses = classNames('euiSuperSelect__popoverPanel', { | |||
[`${popoverClassName}__popoverPanel`]: !!popoverClassName, |
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.
Are the brackets necessary? Shouldn't it just be:
[`${popoverClassName}__popoverPanel`]: !!popoverClassName, | |
`${popoverClassName}__popoverPanel`: !!popoverClassName, |
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 looks like they’re required for dynamic class names: https://github.com/JedWatson/classnames/blob/master/README.md#dynamic-class-names-with-es2015
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.
Huh, I guess this is the first time we've used this pattern. 👍
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, though is there something you're trying to "fix" about the popover panel that this allows you to do? I know this component is super buggy, so just wondering if we need to be aware of something or need an issue.
No, not trying to fix anything with this, we just need it for some ill-advised theming 😭 |
Got it. Whew... 😉 |
Summary
This PR passes
EuiSuperSelect
'spopoverClassName
to the popover's panel if it exists. This change will make it easier to target elements within the popover panel via CSS selectors.Checklist
[ ] This was checked in mobile[ ] This was checked in IE11[ ] This was checked in dark mode[ ] Any props added have proper autodocs[ ] Documentation examples were added[ ] This was checked for breaking changes and labeled appropriately[ ] Jest tests were updated or added to match the most common scenarios[ ] This was checked against keyboard-only and screenreader scenarios[ ] This required updates to Framer X components