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 all commits
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
158 changes: 158 additions & 0 deletions packages/components/src/box-model-overlay/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
# 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 />
</>
);
};
```

`<BoxModelOverlay>` under the hood listens to size and style changes of the target element to update the overlay style automatically using `ResizeObserver` and `MutationObserver`. In some edge cases when the observers aren't picking up the changes, you can use the instance method `update` on the ref of the overlay to update it manually.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly to the previous comment, we usually don't want to explicitly explain how a component works internally

Suggested change
`<BoxModelOverlay>` under the hood listens to size and style changes of the target element to update the overlay style automatically using `ResizeObserver` and `MutationObserver`. In some edge cases when the observers aren't picking up the changes, you can use the instance method `update` on the ref of the overlay to update it manually.
`<BoxModelOverlay>` listens to the target element's size and style changes and updates the overlay style automatically. In some edge cases, in case those changes aren't picked up, you can use the instance method `update` on the `ref` of the overlay to update it manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally, I'd love to see documentation explaining how it works internally so that it feels less magical. (Or else the users have to read the source code which might be inaccessible to some given that it's written in TypeScript.)

I don't think changing the documentation would necessary mean breaking changes though. We didn't make any assumptions about how we use them. The purpose of this whole paragraph is even about when they don't work. I'd argue that the pros outweigh the cons if we expose some internal details, but that's just my opinion 😅 .

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point of view, but given how much WordPress cares about backwards compat, we need to be particularly careful — we treat our public APIs (and what we write in the README) as a contract with the consumers of the component. And therefore, we usually try not to reveal implementation details.

The README, in our views, should be used to understand how to use a component (and not necessarily how it works internally). For those aspects, we think that code comments are better suited (also because they don't represent a "contract", differently from the README).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point! Makes sense to me 👍 .


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

// Update the overlay style manually when `deps` changes.
useEffect( () => {
overlayRef.current.update();
}, [ deps ] );

return (
<BoxModelOverlay showValues={ showValues } ref={ overlayRef }>
<YourComponent />
</BoxModelOverlay>
);
};
```

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