[EuiPopover] Added panelProps prop by akashgp09 · Pull Request #4573 · elastic/eui

akashgp09 · 2021-02-24T11:58:11Z

Summary

This PR Fixes: #4293

Added panelProps prop to EuiPopover

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Edge, and Firefox
Props have proper autodocs and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for the 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

kibanamachine · 2021-02-24T11:58:13Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

myasonik · 2021-02-24T17:28:25Z

Hey @akashgp09! Thanks for this PR but I think there was some confusion in the issue - EuiPanel shouldn't have gotten any new props because what we're hoping to do is pass in the props it already supports through EuiPopover.

I'll try to explain at a high level what we're looking for here and hopefull that clears it up!

So, in src/components/popover/popover.tsx, panelProps should be of type EuiPanelProps.

And then instead of passing passing in <EuiPanel panelProps={panelProps} /> like you're doing, we can spread all the props into the panel like so <EuiPanel {...panelProps} />. It's important for this to be the last property passed in so that these props can serve as an override for anything already set.

akashgp09 · 2021-02-24T23:07:32Z

@myasonik thanks for the review, now it's clear to me ^_^. I will make the changes soon .

myasonik · 2021-02-25T13:37:35Z

jenkins test this

kibanamachine · 2021-02-25T14:02:14Z

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

myasonik

This LGTM!

Will let an EUI team member review/merge.

cchaos · 2021-02-25T16:10:34Z

src/components/panel/panel.tsx

  // Shadows are only allowed when there's a white background (plain)
  const canHaveShadow = color === 'plain';
  const canHaveBorder = color === 'plain' || color === 'transparent';
-


Can you just revert all changes to this file?

cchaos · 2021-02-25T17:32:22Z

src/components/popover/popover.tsx

+              style={this.state.popoverStyles}
+              {...panelProps}>


The only thing about putting this prop at the bottom is that now the user can override any of the props including ones that are absolutely necessary for it display or behave well with a11y. Instead this prop should live at the top of this list and then in the state.popoverStyles account for any possible panelProps?.style and in the panelClasses, add panelProps?.className since these are allowed to be extended but we don't want the user completely overriding the internals.

cchaos · 2021-02-25T21:04:21Z

src/components/popover/popover.tsx

              aria-modal="true"
              aria-describedby={ariaDescribedby}
-              style={this.state.popoverStyles}>
+              style={panelProps?.style || this.state.popoverStyles}>


Sorry this one isn't an or situation. Wherever the state.popoverStyles is being set, you'll need to also add panelProps?.style.

got it 👍

cchaos · 2021-02-25T22:35:31Z

Jenkins, test this

myasonik · 2021-02-26T01:35:24Z

Looks like a type error - not sure how to resolve it from the top of my head

src/components/popover/popover.tsx(764,14): error TS2322: Type '{ children: (Element | undefined)[]; panelRef: (node: HTMLElement | null) => void; className: string; hasShadow: false; paddingSize: "none" | "s" | "m" | "l" | undefined; ... 269 more ...; betaBadgeTitle?: string | undefined; }' is not assignable to type '(IntrinsicAttributes & DisambiguateSet<Divlike, Buttonlike> & Buttonlike & { children?: ReactNode; }) | (IntrinsicAttributes & ... 2 more ... & { ...; })'.
22:41:56   Type '{ children: (Element | undefined)[]; panelRef: (node: HTMLElement | null) => void; className: string; hasShadow: false; paddingSize: "none" | "s" | "m" | "l" | undefined; ... 269 more ...; betaBadgeTitle?: string | undefined; }' is not assignable to type 'Buttonlike'.
22:41:56     Types of property 'onCopy' are incompatible.
22:41:56       Type '((event: ClipboardEvent<HTMLButtonElement>) => void) | ((event: ClipboardEvent<HTMLDivElement>) => void) | undefined' is not assignable to type '((event: ClipboardEvent<HTMLButtonElement>) => void) | undefined'.
22:41:56         Type '(event: ClipboardEvent<HTMLDivElement>) => void' is not assignable to type '(event: ClipboardEvent<HTMLButtonElement>) => void'.
22:41:56           Types of parameters 'event' and 'event' are incompatible.
22:41:56             Type 'ClipboardEvent<HTMLButtonElement>' is not assignable to type 'ClipboardEvent<HTMLDivElement>'.
22:41:56               Property 'align' is missing in type 'HTMLButtonElement' but required in type 'HTMLDivElement'.

chandlerprall · 2021-02-26T16:13:52Z

src/components/popover/popover.tsx

              !ownFocus || !this.state.isOpenStable || this.state.isClosing
            }>
            <EuiPanel
+              {...panelProps}


Passing {...(panelProps as EuiPanelProps)} here will resolve the TypeScript issue mentioned in #4573 (comment) - somewhere between accepting panelProps and forwarding it here TS forgets the value has been validated and appears to intersect the div & button props.

src/components/popover/popover.tsx

…nelProps

akashgp09 · 2021-03-01T17:09:01Z

@cchaos @chandlerprall i have made the changes as suggested. Please let me know if there are any changes required.

cchaos · 2021-03-01T20:12:34Z

Jenkins, test this

kibanamachine · 2021-03-01T20:35:35Z

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

cchaos

👍 Changes look good to me. @chandlerprall Let us know if you're good with the panelProps.style handling.

cchaos · 2021-03-03T15:49:12Z

I'm gonna get this merged in. We can consider deprecating those other props later, but I know they're widely used so this new prop is mostly a ...rest.

Jenkins, test this

kibanamachine · 2021-03-03T16:12:30Z

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

akashgp09 added 2 commits February 24, 2021 17:14

Added panelProps

36ccb55

Added tests for panelProps

2a94b59

Added Cl

acf09b1

akashgp09 added 3 commits February 25, 2021 12:22

changes

bcac269

Merge branch 'master' into panelProps

803a25b

updated cl

bacd5c9

akashgp09 changed the title ~~[EuiPopover,EuiPanel] Added panelProps prop~~ [EuiPopover] Added panelProps prop Feb 25, 2021

myasonik approved these changes Feb 25, 2021

View reviewed changes

cchaos suggested changes Feb 25, 2021

View reviewed changes

added panelProps at the top

a75a26a

akashgp09 requested a review from cchaos February 25, 2021 20:26

cchaos reviewed Feb 25, 2021

View reviewed changes

fixes

94fbbae

akashgp09 requested a review from cchaos February 25, 2021 22:10

Omitting style from panelProps because panelStyle already exists

361f969

chandlerprall reviewed Feb 26, 2021

View reviewed changes

akashgp09 added 2 commits March 1, 2021 22:31

changes

2eba1da

Merge branch 'panelProps' of https://github.com/akashgp09/eui into pa…

1655f5d

…nelProps

cchaos approved these changes Mar 1, 2021

View reviewed changes

Merge branch 'master' into panelProps

986bc10

Merge branch 'master' into panelProps

4be929d

cchaos merged commit 18d7d2b into elastic:master Mar 3, 2021

dependabot bot mentioned this pull request Mar 15, 2021

yarn upgrade: bump @elastic/eui from 31.8.0 to 31.10.0 Noxsios/config-composer#17

Closed

chandlerprall mentioned this pull request Mar 18, 2021

Upgrade EUI to v31.10.0 elastic/kibana#93796

Merged

diegozubieta95 mentioned this pull request Mar 24, 2022

Eui Popover prysmex/ember-eui#111

Merged

7 tasks

Conversation

akashgp09 commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Feb 24, 2021

Uh oh!

myasonik commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akashgp09 commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myasonik commented Feb 25, 2021

Uh oh!

kibanamachine commented Feb 25, 2021

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

cchaos Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cchaos Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akashgp09 Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

cchaos commented Feb 25, 2021

Uh oh!

myasonik commented Feb 26, 2021

Uh oh!

chandlerprall Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akashgp09 commented Mar 1, 2021

Uh oh!

cchaos commented Mar 1, 2021

Uh oh!

kibanamachine commented Mar 1, 2021

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos commented Mar 3, 2021

Uh oh!

kibanamachine commented Mar 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

akashgp09 commented Feb 24, 2021 •

edited

Loading

myasonik commented Feb 24, 2021 •

edited

Loading

akashgp09 commented Feb 24, 2021 •

edited

Loading

cchaos Feb 25, 2021 •

edited

Loading

cchaos Feb 25, 2021 •

edited

Loading