[EuiSuperSelect] Add popoverProps prop #5214

cee-chen · 2021-09-23T21:00:39Z

Summary

While discussing EuiSuperSelect with @scottybollinger, we realized that there isn't a className prop that developers can pass to the actual portalled EuiPopover panel generated by EuiSuperSelect. popoverClassName feels like it should, but actually passes it to the un-portalled wrapping class (instead of to .euiPopover__panel).

This PR solves that by adding a new popoverProps config object to EuiSuperSelect, which passes down almost every prop that EuiPopover would normally take (except for isOpen, which we still want to remain as a top-level API). Users can now simply pass popoverProps={{ className: 'wrapper', panelClassName: 'foo' }} (plus any other popover styling props desired).

Checklist

~~- [ ] Check against all themes for compatibility in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox

Props have proper autodocs and playground toggles (no existing playground for EuiSuperSelect)

- [ ] Added documentation I didn't add extra documentation since there wasn't any for popoverClassName

Checked Code Sandbox works for any docs examples
Added or updated jest tests

~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~

A changelog entry exists and is marked appropriately
Revert eb406ac

Which gives users some way of targeting the portalled popover panel itself, rather than the outer popover wrapper

@scottybollinger

- which also demonstrates the UI behavior @scottybollinger wants

src-docs/src/views/super_select/super_select_complex.js

src/components/form/super_select/super_select.tsx

kibanamachine · 2021-09-23T21:25:45Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5214/

elizabetdev

Thanks @constancecchen,

The new prop looks good and works well. I tested in Chrome, Safari, Edge, and Firefox. 🎉

But the "More complex" demo is already too complex to have there the new prop. So it makes the prop not that discoverable. To improve that, we can create a new section, similar to:
https://elastic.github.io/eui/#/layout/popover#panel-class-name.

The demo can be simple and you can mention both props .popoverClassName and .popoverPanelClassName . This way consumers can easily find how to pass classNames to the popover.

src-docs/src/views/super_select/super_select_complex.js

cee-chen · 2021-09-27T16:01:08Z

@miukimiu The demo/docs changes aren't meant to be in the final merge (they're in the REVERT ME commit). They're just an example to show @scottybollinger how to accomplish the functionality he wants given the new className prop.

elizabetdev · 2021-09-27T16:42:29Z

@constancecchen sorry I missed the REVERT ME commit.

It seems this is a pattern that is going to be used more in the future. 😛

And as @scottybollinger mentioned in Slack:

I have a design that calls for the dropdown items in a Super select to exceed the width of the dropdown itself. Is this possible? I can’t seem to find a way in the docs but wanted to make sure.

I guess more consumers are going to search for this pattern. Just showing the prop might not be enough. So we should create a new section in our docs to ensure consistency.

cee-chen · 2021-09-27T16:55:15Z

I guess more consumers are going to search for this pattern. Just showing the prop might not be enough. So we should create a new section in our docs to ensure consistency.

Sorry to be clear, I don't think this approach should be the final solution to what Scotty wants. See my comment here: #5214 (comment)

Essentially this PR was meant to be a relatively quick workaround that allows him to accomplish the desired UI via CSS override. If we actually want to commonly support EuiSuperSelect's underlying EuiInputPopover being a different width from the attached input, we should 1. actually provide a first-class prop to do so, along the lines of inputPopoverWidth and 2. we'd need to update the underlying EuiInputPopover onResize behavior to be overridden:

eui/src/components/popover/input_popover.tsx

Lines 66 to 75 in e8407ff

    
           const onResize = useCallback(() => { 
        
             if (inputEl) { 
        
               const width = inputEl.getBoundingClientRect().width; 
        
               setInputElWidth(width); 
        
               setPanelWidth(width); 
        
             } 
        
           }, [inputEl, setPanelWidth]); 
        
           useEffect(() => { 
        
             onResize(); 
        
           }, [onResize]);

All of that is way more involved (unit testing, documentation, discussing how this affects other components that use EuiInputPopover and if they also need to support this prop, etc.) than just adding a very simple className prop, which was all this PR was meant to accomplish (since it was a gap in EuiSuperSelect in any case).

Sorry for the long-windedness, but in summary, that's why I don't think this PR needs anything other than prop documentation, and also why I strongly don't think we should be "officially" documenting the use case for Scotty above (rather than just mentioning it as a sub-optimal workaround). Hope that makes sense!

- instead of single static popoverPanelClassName string - this is more flexible and allows users more control of the underlying popover

🤦 whoops

should probably actually test these things..

cee-chen · 2021-09-27T17:50:04Z

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## [`master`](https://github.com/elastic/eui/tree/master)

 - Updated `EuiRangeLevel` `color` property to accept CSS color values ([#5171](https://github.com/elastic/eui/pull/5171))
+- Deprecated  `EuiSuperSelect`'s `popoverClassName`, `isOpen`, & `repositionOnScroll` props in favor of a single `popoverProps` configuration object ([#5214](https://github.com/elastic/eui/pull/5214))


👋 @thompsongl Hey hey, just wanted to highlight that the popoverProps change ended up affecting more than I thought it would! When I reviewed the props we were passing to the underlying EuiInputPopover/EuiPopover, I realized isOpen and repositionOnScroll would need to be deprecated in favor of popoverProps as well (since those get directly passed to the EuiPopover):

LMK what you think. I'm a little nervous because this sorta bumps up the scope of what I wanted the PR to be (a relatively quick workaround) but the actual logic change isn't too complex, just IMO.

Good catch. I actually think isOpen should remain in the top-level API, but repositionOnScroll should move to popoverProps.
It does increase complexity a bit the upgrade path is very simple so I'm ok with the scope creep.

If someone passes isOpen at the top-level but also at the popoverProps level (since isOpen is an accepted EuiPopover prop), which one wins? I assume the top-level prop, but is it confusing to document that / have that behave differently than repositionOnScroll?

I would Omit it from popoverProps in the type definition. I agree with @thompsongl that we don't want to nest this prop becuase it is required to make the select operational.

I'd change it toPartial<CommonProps & Omit<EuiPopoverProps, 'isOpen'>>

The way I think about it is that isOpen is required for basic functionality of EuiSuperSelect and all other popover props are optional and based on context.

Sorry, actually, another thought.... if we don't want popoverProps to be a blanket popover override, then maybe we should omit passing a closePopover fn as well since that's handled by the parent EuiSuperSelect. 🤷

I guess I'm a little worried being selecting about what we do vs don't pass in will lead to weird edge cases with "oh we want to let you customize popover classNames, but not the actual open/close state, or the button ref, or ownFocus/initialFocus, etc..." if that's the case, I wonder if we should basically Pick<> the popover props we do want to allow instead of "all except for ones that we remembered".

Also worth noting all the other EuiPopover props that we set statically in EuiInputPopover that we would now be allowing users to override via popoverProps:

eui/src/components/popover/input_popover.tsx

Lines 108 to 119 in e8407ff

<EuiPopover

ownFocus={false}

button={

<EuiResizeObserver onResize={onResize}>

{(resizeRef) => <div ref={resizeRef}>{input}</div>}

</EuiResizeObserver>

}

buttonRef={inputRef}

panelRef={panelRef}

className={classes}

{...props}

>

Kind of following what I mentioned above, overriding button/buttonRef/panelRef could potentially lead to the component totally breaking 🤷 although it is largely self-inflicted at that point

I was more okay with this when we allowed overriding all EuiPopover props, because that meant the user was essentially completely taking over the state of the popover/dropdown, but I don't think it makes as much sense if we're taking away their control of the popover's isOpen as well 🤔

Definitely could be I'm overthinking this though.

Pick is fine with me to limit the scope of this change but open the door to non-breaking changes later should we need to expand this API.

As far as the non-negotiable props in EuiInputPopover, className can be merged using classnames

After having set this down for awhile and coming back to it, I think I'm overthinking this haha 😅 I'm good with moving forward with just omitting isOpen + clearly documenting that in the prop docs. We can adjust later if we run into issues or other devs voice any confusion/complaints with the API.

LMK if 6edbf98 looks good to y'all as-is!

Looks good to me!

kibanamachine · 2021-09-27T18:11:24Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5214/

cee-chen · 2021-09-27T20:20:21Z

src/components/form/super_select/super_select.tsx

     */
-    isOpen?: boolean;
+    popoverProps?: Partial<CommonProps & Omit<EuiPopoverProps, 'isOpen'>>;


oof, this Omit<> did not parse well 💀

kibanamachine · 2021-09-27T20:42:48Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5214/

elizabetdev

Sorry for the long-windedness, but in summary, that's why I don't think this PR needs anything other than prop documentation

@constancecchen sounds good. 👍🏽

I tested again and LGTM. 🎉

cchaos · 2021-09-28T15:39:16Z

src/components/form/super_select/super_select.tsx


    /**
-     * Controls whether the options are shown. Default: false
+     * Optional props to pass to the underlying [EuiPopover](/#/layout/popover).


Yay for MD renderer!! 🎉

CHANGELOG.md

Co-authored-by: Caroline Horn <[email protected]>

cee-chen · 2021-09-28T15:57:11Z

Thanks y'all, super appreciate all the feedback! I've added the deprecation to #1469 for ~3 months (Jan 2022, rounding up a month).

kibanamachine · 2021-09-28T16:21:20Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5214/

cee-chen added 4 commits September 23, 2021 13:36

[EuiSuperSelect] Add popoverPanelClassName prop

611f6ea

Which gives users some way of targeting the portalled popover panel itself, rather than the outer popover wrapper

Add unit test

79a6ca9

Add changelog entry

0a7869a

[REVERT ME] Add documentation test example

eb406ac

- which also demonstrates the UI behavior @scottybollinger wants

cee-chen requested a review from elizabetdev September 23, 2021 21:00

cee-chen commented Sep 23, 2021

View reviewed changes

src-docs/src/views/super_select/super_select_complex.js Outdated Show resolved Hide resolved

cee-chen commented Sep 23, 2021

View reviewed changes

src/components/form/super_select/super_select.tsx Outdated Show resolved Hide resolved

elizabetdev suggested changes Sep 27, 2021

View reviewed changes

src-docs/src/views/super_select/super_select_complex.js Outdated Show resolved Hide resolved

cee-chen added 5 commits September 27, 2021 10:18

[PR feedback] Add popoverProps obj

7a096b0

- instead of single static popoverPanelClassName string - this is more flexible and allows users more control of the underlying popover

Merge branch 'master' into super-select-popoverClassName

d8f56e2

[REVERT ME] update example with popoverProps

131ec53

Fix bug in popover.className override

a4a266b

🤦 whoops

Better documentation

541a548

should probably actually test these things..

cee-chen commented Sep 27, 2021

View reviewed changes

cee-chen changed the title ~~[EuiSuperSelect] Add popoverPanelClassName prop~~ [EuiSuperSelect] Add popoverProps prop Sep 27, 2021

cee-chen added 2 commits September 27, 2021 13:14

[PR feedback] Remove isOpen prop from being

6edbf98

Revert documentation example

6ed81de

cee-chen commented Sep 27, 2021

View reviewed changes

elizabetdev approved these changes Sep 28, 2021

View reviewed changes

cchaos approved these changes Sep 28, 2021

View reviewed changes

thompsongl approved these changes Sep 28, 2021

View reviewed changes

Constance and others added 2 commits September 28, 2021 08:48

[PR feedback] Changelog copy

f56dc24

Co-authored-by: Caroline Horn <[email protected]>

Merge branch 'master' into super-select-popoverClassName

e4b7904

cee-chen enabled auto-merge (squash) September 28, 2021 15:56

cee-chen mentioned this pull request Sep 28, 2021

[Meta] Deprecation schedule #1469

Open

40 tasks

cee-chen merged commit 91abf40 into elastic:master Sep 28, 2021

cee-chen deleted the super-select-popoverClassName branch September 28, 2021 16:29

elizabetdev mentioned this pull request Sep 28, 2021

[Docs] Links in props tables can exist without their target #5185

Open

cee-chen mentioned this pull request Jan 4, 2022

[EuiSuperSelect] Remove popoverClassName and repositionOnScroll props #5512

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiSuperSelect] Add popoverProps prop #5214

[EuiSuperSelect] Add popoverProps prop #5214

cee-chen commented Sep 23, 2021 •

edited

Loading

kibanamachine commented Sep 23, 2021

elizabetdev left a comment

cee-chen commented Sep 27, 2021 •

edited

Loading

elizabetdev commented Sep 27, 2021

cee-chen commented Sep 27, 2021 •

edited

Loading

cee-chen Sep 27, 2021 •

edited

Loading

thompsongl Sep 27, 2021

cee-chen Sep 27, 2021 •

edited

Loading

cchaos Sep 27, 2021

thompsongl Sep 27, 2021

cee-chen Sep 27, 2021 •

edited

Loading

cee-chen Sep 27, 2021 •

edited

Loading

thompsongl Sep 27, 2021

cee-chen Sep 27, 2021 •

edited

Loading

thompsongl Sep 28, 2021

kibanamachine commented Sep 27, 2021

cee-chen Sep 27, 2021

kibanamachine commented Sep 27, 2021

elizabetdev left a comment

cchaos Sep 28, 2021

cee-chen commented Sep 28, 2021

kibanamachine commented Sep 28, 2021

	<EuiPopover
	ownFocus={false}
	button={
	<EuiResizeObserver onResize={onResize}>
	{(resizeRef) => <div ref={resizeRef}>{input}</div>}
	</EuiResizeObserver>
	}
	buttonRef={inputRef}
	panelRef={panelRef}
	className={classes}
	{...props}
	>

[EuiSuperSelect] Add popoverProps prop #5214

[EuiSuperSelect] Add popoverProps prop #5214

Conversation

cee-chen commented Sep 23, 2021 • edited Loading

Summary

Checklist

kibanamachine commented Sep 23, 2021

elizabetdev left a comment

Choose a reason for hiding this comment

cee-chen commented Sep 27, 2021 • edited Loading

elizabetdev commented Sep 27, 2021

cee-chen commented Sep 27, 2021 • edited Loading

cee-chen Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

thompsongl Sep 27, 2021

Choose a reason for hiding this comment

cee-chen Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

cchaos Sep 27, 2021

Choose a reason for hiding this comment

thompsongl Sep 27, 2021

Choose a reason for hiding this comment

cee-chen Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

cee-chen Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

thompsongl Sep 27, 2021

Choose a reason for hiding this comment

cee-chen Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

thompsongl Sep 28, 2021

Choose a reason for hiding this comment

kibanamachine commented Sep 27, 2021

cee-chen Sep 27, 2021

Choose a reason for hiding this comment

kibanamachine commented Sep 27, 2021

elizabetdev left a comment

Choose a reason for hiding this comment

cchaos Sep 28, 2021

Choose a reason for hiding this comment

cee-chen commented Sep 28, 2021

kibanamachine commented Sep 28, 2021

cee-chen commented Sep 23, 2021 •

edited

Loading

cee-chen commented Sep 27, 2021 •

edited

Loading

cee-chen commented Sep 27, 2021 •

edited

Loading

cee-chen Sep 27, 2021 •

edited

Loading

cee-chen Sep 27, 2021 •

edited

Loading

cee-chen Sep 27, 2021 •

edited

Loading

cee-chen Sep 27, 2021 •

edited

Loading

cee-chen Sep 27, 2021 •

edited

Loading