Skip to content
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.

Sign up for GitHub

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

[EuiSuperSelect] Add popoverProps prop #5214

Merged
merged 13 commits into from
Sep 28, 2021
Merged

[EuiSuperSelect] Add popoverProps prop #5214

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
611f6ea
[EuiSuperSelect] Add `popoverPanelClassName` prop
cee-chen Sep 23, 2021
79a6ca9
Add unit test
cee-chen Sep 23, 2021
0a7869a
Add changelog entry
cee-chen Sep 23, 2021
eb406ac
[REVERT ME] Add documentation test example
cee-chen Sep 23, 2021
7a096b0
[PR feedback] Add popoverProps obj
cee-chen Sep 27, 2021
d8f56e2
Merge branch 'master' into super-select-popoverClassName
cee-chen Sep 27, 2021
131ec53
[REVERT ME] update example with popoverProps
cee-chen Sep 27, 2021
a4a266b
Fix bug in popover.className override
cee-chen Sep 27, 2021
541a548
Better documentation
cee-chen Sep 27, 2021
6edbf98
[PR feedback] Remove `isOpen` prop from being
cee-chen Sep 27, 2021
6ed81de
Revert documentation example
cee-chen Sep 27, 2021
f56dc24
[PR feedback] Changelog copy
constancecchen Sep 28, 2021
e4b7904
Merge branch 'master' into super-select-popoverClassName
constancecchen Sep 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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))
Copy link
Contributor Author

@cee-chen cee-chen Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 @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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@cee-chen cee-chen Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@cee-chen cee-chen Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor Author

@cee-chen cee-chen Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cee-chen cee-chen Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!


**Bug fixes**

Expand Down
28 changes: 21 additions & 7 deletions src-docs/src/views/super_select/super_select_complex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,26 @@ export default () => {
};

return (
<EuiSuperSelect
options={options}
valueOfSelected={value}
onChange={(value) => onChange(value)}
itemLayoutAlign="top"
hasDividers
/>
<>
<style>
{`.wrapper {
width: 150px;
}
.dropdown {
width: 450px !important;
}`}
</style>
<EuiSuperSelect
options={options}
valueOfSelected={value}
onChange={(value) => onChange(value)}
itemLayoutAlign="top"
hasDividers
popoverProps={{
className: 'wrapper',
panelClassName: 'dropdown',
}}
/>
</>
);
};
228 changes: 228 additions & 0 deletions src/components/form/super_select/__snapshots__/super_select.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,234 @@ exports[`EuiSuperSelect props options are rendered when select is open 1`] = `
</div>
`;

exports[`EuiSuperSelect props renders popoverProps on the underlying EuiPopover 1`] = `
<EuiSuperSelect
compressed={false}
fullWidth={false}
hasDividers={false}
isInvalid={false}
isLoading={false}
onChange={[Function]}
options={
Array [
Object {
"inputDisplay": "Option #1",
"value": "1",
},
Object {
"inputDisplay": "Option #2",
"value": "2",
},
]
}
popoverProps={
Object {
"className": "goes-on-outermost-wrapper",
"isOpen": false,
"panelClassName": "goes-on-popover-panel",
"repositionOnScroll": true,
}
}
valueOfSelected="2"
>
<EuiInputPopover
anchorPosition="downLeft"
attachToAnchor={true}
className="euiSuperSelect goes-on-outermost-wrapper"
closePopover={[Function]}
display="block"
fullWidth={false}
input={
<EuiSuperSelectControl
className=""
compressed={false}
fullWidth={false}
isInvalid={false}
isLoading={false}
onClick={[Function]}
onKeyDown={[Function]}
options={
Array [
Object {
"inputDisplay": "Option #1",
"value": "1",
},
Object {
"inputDisplay": "Option #2",
"value": "2",
},
]
}
value="2"
/>
}
isOpen={false}
panelClassName="goes-on-popover-panel"
panelPaddingSize="none"
repositionOnScroll={true}
>
<EuiPopover
anchorPosition="downLeft"
attachToAnchor={true}
button={
<EuiResizeObserver
onResize={[Function]}
>
[Function]
</EuiResizeObserver>
}
buttonRef={[Function]}
className="euiInputPopover euiSuperSelect goes-on-outermost-wrapper"
closePopover={[Function]}
display="block"
hasArrow={true}
isOpen={false}
ownFocus={false}
panelClassName="goes-on-popover-panel"
panelPaddingSize="none"
panelRef={[Function]}
repositionOnScroll={true}
>
<EuiOutsideClickDetector
onOutsideClick={[Function]}
>
<div
className="euiPopover euiPopover--anchorDownLeft euiPopover--displayBlock euiInputPopover euiSuperSelect goes-on-outermost-wrapper"
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseUp={[Function]}
onTouchEnd={[Function]}
onTouchStart={[Function]}
>
<div
className="euiPopover__anchor"
>
<EuiResizeObserver
onResize={[Function]}
>
<div>
<EuiSuperSelectControl
className=""
compressed={false}
fullWidth={false}
isInvalid={false}
isLoading={false}
onClick={[Function]}
onKeyDown={[Function]}
options={
Array [
Object {
"inputDisplay": "Option #1",
"value": "1",
},
Object {
"inputDisplay": "Option #2",
"value": "2",
},
]
}
value="2"
>
<input
type="hidden"
value="2"
/>
<EuiFormControlLayout
compressed={false}
fullWidth={false}
icon={
Object {
"side": "right",
"type": "arrowDown",
}
}
isLoading={false}
>
<div
className="euiFormControlLayout"
>
<div
className="euiFormControlLayout__childrenWrapper"
>
<EuiScreenReaderOnly>
<span
className="euiScreenReaderOnly"
id="generated-id"
>
<EuiI18n
default="Select an option: {selectedValue}, is selected"
token="euiSuperSelectControl.selectAnOption"
values={
Object {
"selectedValue": "Option #2",
}
}
>
Select an option: Option #2, is selected
</EuiI18n>
</span>
</EuiScreenReaderOnly>
<button
aria-haspopup="true"
aria-labelledby="generated-id"
className="euiSuperSelectControl"
onClick={[Function]}
onKeyDown={[Function]}
type="button"
>
Option #2
</button>
<EuiFormControlLayoutIcons
compressed={false}
icon={
Object {
"side": "right",
"type": "arrowDown",
}
}
isLoading={false}
>
<div
className="euiFormControlLayoutIcons euiFormControlLayoutIcons--right"
>
<EuiFormControlLayoutCustomIcon
size="m"
type="arrowDown"
>
<span
className="euiFormControlLayoutCustomIcon"
>
<EuiIcon
aria-hidden="true"
className="euiFormControlLayoutCustomIcon__icon"
size="m"
type="arrowDown"
>
<span
aria-hidden="true"
className="euiFormControlLayoutCustomIcon__icon"
data-euiicon-type="arrowDown"
size="m"
/>
</EuiIcon>
</span>
</EuiFormControlLayoutCustomIcon>
</div>
</EuiFormControlLayoutIcons>
</div>
</div>
</EuiFormControlLayout>
</EuiSuperSelectControl>
</div>
</EuiResizeObserver>
</div>
</div>
</EuiOutsideClickDetector>
</EuiPopover>
</EuiInputPopover>
</EuiSuperSelect>
`;

exports[`EuiSuperSelect props select component is rendered 1`] = `
<div
class="euiPopover euiPopover--anchorDownLeft euiPopover--displayBlock euiInputPopover euiSuperSelect"
Expand Down
18 changes: 18 additions & 0 deletions src/components/form/super_select/super_select.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,5 +161,23 @@ describe('EuiSuperSelect', () => {

expect(takeMountedSnapshot(component)).toMatchSnapshot();
});

test('renders popoverProps on the underlying EuiPopover', () => {
const component = mount(
<EuiSuperSelect
options={options}
valueOfSelected="2"
onChange={() => {}}
popoverProps={{
className: 'goes-on-outermost-wrapper',
panelClassName: 'goes-on-popover-panel',
repositionOnScroll: true,
isOpen: false,
}}
/>
);

expect(component).toMatchSnapshot();
});
});
});
32 changes: 25 additions & 7 deletions src/components/form/super_select/super_select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
EuiSuperSelectControlProps,
EuiSuperSelectOption,
} from './super_select_control';
import { EuiInputPopover } from '../../popover';
import { EuiInputPopover, EuiPopoverProps } from '../../popover';
import {
EuiContextMenuItem,
EuiContextMenuItemLayoutAlignment,
Expand Down Expand Up @@ -68,21 +68,34 @@ export type EuiSuperSelectProps<T extends string> = CommonProps &
*/
itemLayoutAlign?: EuiContextMenuItemLayoutAlignment;

/**
* Props to pass to the underlying [EuiPopover](/#/layout/popover). Allows
* fine-grained control of the popover dropdown menu, including `isOpen` state,
* `repositionOnScroll` for EuiSuperSelects used within scrollable containers,
* and customizing popover panel styling.
*/
popoverProps?: Partial<CommonProps & EuiPopoverProps>;

/**
* Applied to the outermost wrapper (popover)
*
* **DEPRECATED: Use `popoverProps.className` instead (will take precedence over this prop if set).**
*/
popoverClassName?: string;

/**
* Controls whether the options are shown. Default: false
*
* **DEPRECATED: Use `popoverProps.isOpen` instead (will take precedence over this prop if set).**
*/
isOpen?: boolean;

/**
* When `true`, the popover's position is re-calculated when the user
* scrolls, this supports having fixed-position popover anchors. This value is passed
* to the EuiInputPopover component. When nesting an `EuiSuperSelect` in a scrollable container,
* scrolls. When nesting an `EuiSuperSelect` in a scrollable container,
* `repositionOnScroll` should be `true`
*
* **DEPRECATED: Use `popoverProps.repositionOnScroll` instead (will take precedence over this prop if set).**
*/
repositionOnScroll?: boolean;
};
Expand Down Expand Up @@ -259,12 +272,16 @@ export class EuiSuperSelect<T extends string> extends Component<
itemLayoutAlign,
fullWidth,
popoverClassName,
popoverProps,
compressed,
repositionOnScroll,
...rest
} = this.props;

const popoverClasses = classNames('euiSuperSelect', popoverClassName);
const popoverClasses = classNames(
'euiSuperSelect',
popoverProps?.className ?? popoverClassName
);

const buttonClasses = classNames(
{
Expand Down Expand Up @@ -321,13 +338,14 @@ export class EuiSuperSelect<T extends string> extends Component<

return (
<EuiInputPopover
className={popoverClasses}
input={button}
isOpen={isOpen || this.state.isPopoverOpen}
closePopover={this.closePopover}
panelPaddingSize="none"
fullWidth={fullWidth}
repositionOnScroll={repositionOnScroll}
{...popoverProps}
className={popoverClasses}
input={button}
fullWidth={fullWidth}
>
<EuiScreenReaderOnly>
<p role="alert">
Expand Down