Components: promote g2 Popover as Flyout
#32197
Conversation
ciampo
added
[Package] Components
|
Size Change: +12.2 kB (+1%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
This isn't really necessary. If you end up with extra time it's fine to do but according to the strategy laid out in this make post about TypeScript in Gutenberg we shouldn't be re-writing to TypeScript unless it's necessary for expressing the types (which in this case it isn't). We haven't decided what to do about |
|
Can we keep the name @diegohaz used such approach for the gutenberg/packages/components/src/toolbar/index.js Lines 29 to 37 in 152420f |
I'm quite new to the g2 components work, and so I don't really have much context to give a good answer to this question. I based my choice on what was agreed with @sarayourfriend (and on previous conversations like this comment), but I'm definitely also up for adopting the same approach used for the cc @griffbrad |
I think this is more complicated than the Toolbar component. The difference between the legacy toolbar and the current accessible toolbar enabled by the Also, another motivation to keep the Maybe we can discuss more the name though, like |
@diegohaz, thank you for feedback. We can have completely new name if that makes more sense here. The old |
|
Agreed that the name |
ciampo
force-pushed
the
feat/components-promote-popover
branch
from
ac2da4e to
3009f50
Compare
|
I just rebased this PR to include a few changes that got recently merged on
@sarayourfriend , @gziolo and @diegohaz — let's discuss the next steps for this PR:
|
I think a deprecation warning is enough, but I would only do that after we make sure this component is stable and covers all the current |
Alright,
This also makes sense. It looks like the next steps could be:
|
ciampo
changed the title
Popover as AccessiblePopoverPopover as Flyout
ciampo
force-pushed
the
feat/components-promote-popover
branch
from
953872b to
3598b73
Compare
|
|
||
| /** | ||
| * | ||
| * @param {import('../../ui/context').PolymorphicComponentProps<import('../types').ContentProps, 'div', false>} props |
There was a problem hiding this comment.
The refactoring done in 3598b73, (where we wrapped ReakitPopover directly into the styled function), caused a typescript error around the as prop on < FlyoutContentView />.
For this reason, FlyoutContent (and Flyout) have been marked as non-polymorphic
There was a problem hiding this comment.
Out of curiosity, what's the context behind refactoring the css calls to styled.div and then to styled( ReakitPopover )? I remember we've had some conversations about that, but I can't remember exactly the reasoning behind it. cc @sarayourfriend
There was a problem hiding this comment.
Basically @emotion/css does not hook into the Emotion cache so it's not possible for us to use it's css function because we need to hook into the cache to support iframes.
@emotion/styled does hook into the CacheProvider so it is able to support iframes.
|
This PR is now ready for a full review! I've also updated the description to match the latest changes. @sarayourfriend and @diegohaz , would you mind taking another look? Thank you! |
ciampo
force-pushed
the
feat/components-promote-popover
branch
from
35dfc6b to
2b5d016
Compare
|
I've rebased the PR and all tests are now passing. From what I gathered, we seem to agree that the next steps are:
If this is the case, we need a final review/approval before this PR can be merged. |
sarayourfriend
left a comment
sarayourfriend
left a comment
There was a problem hiding this comment.
LGTM! 🚀 Storybook tests well and the code makes sense. Thanks for your persistance on this very long one!
* Move `components/src/ui/popover` to `components/src/accessible-popover` * Add to Docs manifest * Add to tsconfig.json * Rename context-related variables from Popover* to AccessiblePopover* This ensures consistency in the folder, even if the component name is going to change * Rename `PopoverContext` type to `Context` * Rename `PopoverContent` to `AccessiblePopoverContent` * Move `component.js` to component-specific folder * Move `AccessibilePopover` logic to separate hook * Move `content.js` to component-specific folder * Move `AccessibilePopoverContent` logic to separate hook * Rename `AccessiblePopover` to `Flyout` * Rename zIndex const from `Popover` to `Flyout` * Do not export FlyoutContent component * Fix imports in example * Rename `popover` to `flyoutState` * Refactor to `styled` approach * Wrap `ReakitPopover` in `FlyoutContentView` and set `maxWidth` in inline styles * auto-format * Add Props documentation to Flyout * Mark `Flyout` as non-polymorphic * Add documentation to `FlyoutContent`s README * Update snapshots * Remove FlyoutContext exports * Do not export FlyoutContent`s documentation * Delete FlyoutContent`s README
Description
Popovercomponent from thecomponents/src/ui/folder into thecomponents/srcfolderFlyoutFlyoutandFlyoutContentcomponents.styledapproachtsconfigand the docs manifestNext steps in follow-up PRs:
Flyoutcomponent to make sure it is stable and covers all use cases of the current (legacy)Popover. A few ways of conducting this investigation are:Popoverwith the newFlyoutPopoverHow has this been tested?
@wordpress/componentspackage builds correctlyFlyoutcomponent in locally-run Storybook and make sure it behaves the same way as the g2Popovercomponent on the production branchScreenshots
Types of changes
Refactor (moving and partially rewriting files)
Checklist:
*.native.jsfiles for terms that need renaming or removal).