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

Add <BoxModelOverlay> component #40253

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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: 2 additions & 2 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ const majorMinorRegExp =
*/
const developmentFiles = [
'**/benchmark/**/*.js',
'**/@(__mocks__|__tests__|test)/**/*.js',
'**/@(storybook|stories)/**/*.js',
'**/@(__mocks__|__tests__|test)/**/*.[tj]s?(x)',
'**/@(storybook|stories)/**/*.[tj]s?(x)',
Comment on lines +29 to +30
Copy link
Member Author

Choose a reason for hiding this comment

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

To support writing tests in .ts and .tsx.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're still working on the best way to enable TypeScript unit tests in Jest, for the time being we should write unit tests in JS (see ongoing work in #39436)

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: #39436 has been merged, we should be able to remove the changes to this file in this PR, and rebase on top of trunk to support TypeScript tests

'packages/babel-preset-default/bin/**/*.js',
];

Expand Down
6 changes: 6 additions & 0 deletions docs/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,12 @@
"markdown_source": "../packages/components/src/box-control/README.md",
"parent": "components"
},
{
"title": "BoxModelOverlay",
"slug": "box-model-overlay",
"markdown_source": "../packages/components/src/box-model-overlay/README.md",
"parent": "components"
},
{
"title": "ButtonGroup",
"slug": "button-group",
Expand Down
139 changes: 139 additions & 0 deletions packages/components/src/box-model-overlay/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
# BoxModelOverlay

<div class="callout callout-alert">
This feature is still experimental. “Experimental” means this is an early implementation subject to drastic and breaking changes.
</div>

`<BoxModelOverlay>` component shows a visual overlay of the [box model](https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks/The_box_model) (currently only paddings and margins are available) on top of the target element. This is often accompanied by the `<BoxControl>` component to show a preview of the styling changes in the editor.
Copy link
Contributor

Choose a reason for hiding this comment

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

(currently only paddings and margins are available)

If we're making references to the box model for this component, I believe that it should fully support borders as well, otherwise it would be a bit weird?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it should, but borders are usually already visible to users. We can discuss this further in another PR though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, let's address this in a follow-up PR


## Usage

Wrap `<YourComponent>` with `<BoxModelOverlay>` with the `showValues` prop.
Note that `<YourComponent>` should accept `ref` for `<BoxModelOverlay>` to automatically inject into.

```jsx
import { __experimentalBoxModelOverlay as BoxModelOverlay } from '@wordpress/components';

// Show all overlays and all sides.
const showValues = {
margin: {
top: true,
right: true,
bottom: true,
left: true,
},
padding: {
top: true,
right: true,
bottom: true,
left: true,
},
};

const Example = () => {
return (
<BoxModelOverlay showValues={ showValues }>
<YourComponent />
</BoxModelOverlay>
);
};
```

You can also use the `targetRef` prop to manually pass the ref to `<BoxModelOverlay>` for more advanced usage. This is useful if you need to control where the overlay is rendered or need special handling for the target's `ref`.

```jsx
const Example = () => {
const targetRef = useRef();

return (
<>
<YourComponent ref={ targetRef } />

<BoxModelOverlay
showValues={ showValues }
targetRef={ targetRef }
/>
</>
);
};
```

`<BoxModelOverlay>` internally uses [`Popover`](https://github.com/WordPress/gutenberg/blob/HEAD/packages/components/src/popover/README.md) to position the overlay. This means that you can use `<Popover.Slot>` to alternatively control where the overlay is rendered.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should expose how internally a component works — hiding these implementation details would allow us to make changes in a non-breaking way in the future (for example, what if at some point we change how Popover works? Or what if we decided to switch to Flyout ?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see this as an implementation detail though. This part is a fundamental part of the component. If we change to use Flyout, then <Popover.Slot> will no longer work, so we'll have to make breaking changes either way. If we change how <Popover> works... then it's already a breaking change 😆 ?

Anyway, this is still an experimental component and can change anytime. Maybe we'll like to deprecate this <Popover.Slot> usage and it's fine to do so without introducing breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right — even if it's not ideal, given that <Popover.Slot> is currently a fundamental part of how BoxModalOverlay works, it's fine to keep this part of the README as-is.

Thank you for the explanation!


```jsx
const Example = () => {
return (
<>
<BoxModelOverlay showValues={ showValues }>
<YourComponent />
</BoxModelOverlay>

<Popover.Slot />
</>
);
};
```

Here's an example of using it with `<BoxControl>`:

```jsx
const Example = () => {
const [ values, setValues ] = useState( {
top: '50px',
right: '10%',
bottom: '50px',
left: '10%',
} );
const [ showValues, setShowValues ] = useState( {} );

return (
<>
<BoxControl
label="Padding"
values={ values }
onChange={ ( nextValues ) => setValues( nextValues ) }
onChangeShowVisualizer={ setShowValues }
/>

<BoxModelOverlay showValues={ showValues }>
<div
style={ {
height: 300,
width: 300,
paddingTop: values.top,
paddingRight: values.right,
paddingBottom: values.bottom,
paddingLeft: values.left,
} }
/>
</BoxModelOverlay>
</>
);
};
```

## Props

Additional props not listed below will be passed to the underlying `Popover` component.

### `showValues`

Controls which overlays and sides are visible. Currently the only properties supported are `margin` and `padding`, each with four sides (`top`, `right`, `bottom`, `left`).

- Type: `Object`
- Required: Yes
- Default: `{}`

### `children`

A single React element to rendered as the target. It should implicitly accept `ref` to be passed in.

- Type: `React.ReactElement`
- Required: Yes if `targetRef` is not passed

### `targetRef`

A ref object for the target element.

- Type: `Ref<HTMLElement>`
- Required: Yes if `children` is not passed
Comment on lines +146 to +158
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we'd like to avoid having conditional logic in the types (see this recent comment by @mirka ) — can we find an alternative here?

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 feels backward to me, though. I don't think we should change how a component works because the tooling doesn't support it; we should update the tooling to support it instead. Is there any other reason why we shouldn't use conditional props?

Copy link
Member

Choose a reason for hiding this comment

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

Right, we're definitely not saying that docgen limitations should be the driving factor for designing component APIs. The general sentiment of wanting to avoid conditional props was triggered by situations where it just added to the maintenance burden. It's a slippery slope that can easily complicate itself over time into logic that is hard to hold in your head. This is nonetheless a cognitive burden that slows down dev work, no matter how well TypeScript can programatically enforce the conditions.

So it's actually not just about docgen tooling, but humans and human-facing docs in general, because there's still a limit to how coherently you can describe conditional props even in a handwritten README.md. It may seem simple enough now, but we also have to consider how those conditions may be forced to evolve/complicate over time, and how it adds to the cognitive load of both maintainers and consumers.

If it's absolutely worth those costs, sure! But it's good to consider whether there is a simpler, human-friendly API we can land on (e.g. remove the children usage, or export them as separate components).

Loading