-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Anyway, this is still an experimental component and can change anytime. Maybe we'll like to deprecate this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right — even if it's not ideal, given that 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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😅 . There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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