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

SlotFill: Refactor <Slot bubblesVirtually /> #53272

Merged
merged 5 commits into from
Aug 23, 2023
Merged

SlotFill: Refactor <Slot bubblesVirtually /> #53272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2010a1e
remove children form bubbleVisually slot.
torounit Aug 2, 2023
c596785
update readme for `Slot`
torounit Aug 2, 2023
70b5607
Use "as" as is.
torounit Aug 2, 2023
2b8c688
update changelog
torounit Aug 2, 2023
362b6d4
Clarify changelog.
torounit Aug 21, 2023
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
4 changes: 3 additions & 1 deletion packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

- `SandBox`: Fix the cleanup method in useEffect ([#53796](https://github.com/WordPress/gutenberg/pull/53796)).

### Internal
- `SlotFill`: Do not render children when using `<Slot bubblesVirtually />`. ([#53272](https://github.com/WordPress/gutenberg/pull/53272))

### Enhancements

- `ProgressBar`: Add transition to determinate indicator ([#53877](https://github.com/WordPress/gutenberg/pull/53877)).
Expand Down Expand Up @@ -46,7 +49,6 @@

- `ColorPalette`, `BorderControl`: Don't hyphenate hex value in `aria-label` ([#52932](https://github.com/WordPress/gutenberg/pull/52932)).
- `MenuItemsChoice`, `MenuItem`: Support a `disabled` prop on a menu item ([#52737](https://github.com/WordPress/gutenberg/pull/52737)).
- `TabPanel`: Introduce a new version of `TabPanel` with updated internals and improved adherence to ARIA guidance on `tabpanel` focus behavior while maintaining the same functionality and API surface.([#52133](https://github.com/WordPress/gutenberg/pull/52133)).
Copy link
Contributor

Choose a reason for hiding this comment

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

@torounit Just noticed that this PR removed a CHANGELOG entry — was that on purpose? If not, could you add it back (maybe as part of #51350) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a mistake. Commit to PR #51350.


### Bug Fix

Expand Down
10 changes: 5 additions & 5 deletions packages/components/src/slot-fill/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ Both `Slot` and `Fill` accept a `name` string prop, where a `Slot` with a given

`Slot` with `bubblesVirtually` set to true also accept an optional `className` to add to the slot container.

`Slot` accepts an optional `children` function prop, which takes `fills` as a param. It allows you to perform additional processing and wrap `fills` conditionally.
`Slot` **without** `bubblesVirtually` accepts an optional `children` function prop, which takes `fills` as a param. It allows you to perform additional processing and wrap `fills` conditionally.

_Example_:

Expand Down Expand Up @@ -103,14 +103,14 @@ const ToolbarItem = () => (
</Fill>
);

const Toolbar = () =>
const hideToolbar() => {
const Toolbar = () => {
const hideToolbar = () => {
console.log( 'Hide toolbar' );
}
};
return (
<div className="toolbar">
<Slot fillProps={ { hideToolbar } } />
</div>
);
);
};
```
22 changes: 17 additions & 5 deletions packages/components/src/slot-fill/bubbles-virtually/slot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,20 @@ import { useMergeRefs } from '@wordpress/compose';
/**
* Internal dependencies
*/
import { View } from '../../view';
import SlotFillContext from './slot-fill-context';

function Slot(
{ name, fillProps = {}, as: Component = 'div', ...props },
forwardedRef
) {
function Slot( props, forwardedRef ) {
const {
name,
fillProps = {},
as,
// `children` is not allowed. However, if it is passed,
// it will be displayed as is, so remove `children`.
children,
...restProps
} = props;

const { registerSlot, unregisterSlot, ...registry } =
useContext( SlotFillContext );
const ref = useRef();
Expand All @@ -41,7 +49,11 @@ function Slot(
} );

return (
<Component ref={ useMergeRefs( [ forwardedRef, ref ] ) } { ...props } />
<View
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to use the View component here? Seems to me that the original div was fine, and that now we're just adding more indirection and complexity. Is there any tangible benefit from the fact that the div is now styled with Emotion?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jsnajdr Thanks.

In the original Pull Request, I changed to View to avoid error: TS2590, which I am trying to address in this PR.

#51350 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I see, so it's not because of View functionality, but to solve a typing problem. Hopefully in future, when the rename from JS to TS is done, we can find a way to go back to simple div.

as={ as }
ref={ useMergeRefs( [ forwardedRef, ref ] ) }
{ ...restProps }
/>
);
}

Expand Down
16 changes: 12 additions & 4 deletions packages/components/src/slot-fill/test/__snapshots__/slot.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,16 @@ exports[`Slot bubblesVirtually true should subsume another slot by the same name
<div
data-position="first"
>
<div />
<div
class="emotion-0 emotion-1"
/>
</div>
<div
data-position="second"
>
<div>
<div
class="emotion-0 emotion-1"
>
Content
</div>
</div>
Expand All @@ -62,7 +66,9 @@ exports[`Slot bubblesVirtually true should subsume another slot by the same name
<div
data-position="second"
>
<div>
<div
class="emotion-0 emotion-1"
>
Content
</div>
</div>
Expand Down Expand Up @@ -187,7 +193,9 @@ exports[`Slot should render in expected order when fills unmounted 1`] = `
exports[`Slot should warn without a Provider 1`] = `
<div>
<div>
<div />
<div
class="emotion-0 emotion-1"
/>
</div>
</div>
`;