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.

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

Components: Allow style prop on Popover #64489

Merged
merged 9 commits into from
Aug 15, 2024

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Aug 14, 2024

What?

In #64321 I have a use case for using the style prop on the Popover component (see 'Why?').

I discovered that you can use the style prop, but doing so clears the style properties set by floating-ui that position the popover because of the way contentProps.style (contentProps collects the 'rest' of the component's attributes) overrides animationProps.style when the attributes are spread onto the component:

{ ...animationProps }
{ ...contentProps }

In this PR I'm making the style prop work correctly.

How?

Spreads the style prop into the animationProps.style property, giving it lower precedence than other 'built-in' styles so that it isn't possible to break the popover's innate positioning.

Why?

#64321 renders a popover inside the editor canvas iframe and requires overriding margin (which is added by the block layout system) and zIndex (one of the popover's own styles). Due to the popover being inside the iframe, regular scss styles can't be used (they're not loaded in the iframe), so inline styles seem to be the obvious option.

There are possible alternatives:

  • Add a prop that removes zIndex from the iframe (e.g. noZIndex), or allows setting a custom z index. The margin will still be overriden, but I can solve that by wrapping the popover in a div that has style={{margin:0}} specified. I'm not sure this solution is better than the one in this PR though, it adds an extra prop that's of limited use.
  • Render a style sheet within the canvas iframe. I'm reluctant when it comes to this option as it adds much more complexity compared to using an inline style, which is simple and has no specificity issues.

Testing Instructions

A couple of unit tests have been added. I wasn't sure about adding a story for this - I thought it might be best to leave this prop undocumented just like the other html attributes that can be added via ...contentProps.

@talldan talldan added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system labels Aug 14, 2024
@talldan talldan self-assigned this Aug 14, 2024
Copy link

github-actions bot commented Aug 14, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: talldan <[email protected]>
Co-authored-by: mirka <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Makes sense, and the tests look great! Thanks 🚀

Comment on lines 163 to 168
/**
* Inline styles to apply to the popover element.
*
* @default undefined
*/
style?: React.CSSProperties;
Copy link
Member

Choose a reason for hiding this comment

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

Technically we don't have to list this type explicitly, because it's already included as a div prop via WordPressComponentProps, and excluded from the MotionProps omitting.

And I guess this comment can be a bit misleading:

  // To avoid overlaps between the standard HTML attributes and the props
  // expected by `framer-motion`, omit all framer motion props from popover
  // props (except for `animate`, `children`, and `style`, which are
  // re-defined in `PopoverProps`).

Maybe "except for animate and children which are re-defined in PopoverProps, and style which is merged safely"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good - done in c811da6. Thanks for the quick review!

Copy link

Flaky tests detected in c811da6.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10397832171
📝 Reported issues:

@talldan talldan merged commit c8eaf5c into trunk Aug 15, 2024
62 of 63 checks passed
@talldan talldan deleted the update/allow-style-prop-on-popover branch August 15, 2024 03:49
@github-actions github-actions bot added this to the Gutenberg 19.1 milestone Aug 15, 2024
@fabiankaegy fabiankaegy mentioned this pull request Oct 1, 2024
97 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants