Add <BoxModelOverlay> component
#40253
Conversation
|
Size Change: +2.06 kB (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
kevin940726
changed the title
<BoxModelOverlay> component
| '**/@(__mocks__|__tests__|test)/**/*.[tj]s?(x)', | ||
| '**/@(storybook|stories)/**/*.[tj]s?(x)', |
There was a problem hiding this comment.
To support writing tests in .ts and .tsx.
There was a problem hiding this comment.
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.
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
| export { | ||
| BoxModelOverlayProps, | ||
| BoxModelOverlayPropsWithChildren, | ||
| BoxModelOverlayPropsWithTargetRef, | ||
| BoxModelOverlayHandle, | ||
| }; |
There was a problem hiding this comment.
These are only exported for the tests for now. Not sure how we can export them to the package level when we're still using .js in our entry file.
| window.ResizeObserver = undefined; | ||
| } ); | ||
|
|
||
| it( 'renders the overlay visible with the children prop', () => { |
There was a problem hiding this comment.
Honestly, these tests aren't that helpful for this particular component, since JSDOM doesn't render anything visually.
Perhaps we can set up an infrastructure to write unit tests to run in Playwright in the future.
There was a problem hiding this comment.
Perhaps we can set up an infrastructure to write unit tests to run in Playwright in the future.
This is something that's we've discussed in the past, and flagging limits like you've just done definitely helps justifying that effort (cc @mirka )
| // Copied from Chrome's DevTools: https://github.com/ChromeDevTools/devtools-frontend/blob/088a8f175bd58f2e0e2d492e991a3253124d7c11/front_end/core/common/Color.ts#L931 | ||
| const MARGIN_COLOR = 'rgba( 246, 178, 107, 0.66 )'; | ||
| // Copied from Chrome's DevTools: https://github.com/ChromeDevTools/devtools-frontend/blob/088a8f175bd58f2e0e2d492e991a3253124d7c11/front_end/core/common/Color.ts#L927 | ||
| const PADDING_COLOR = 'rgba( 147, 196, 125, 0.55 )'; |
There was a problem hiding this comment.
These values mimic the styles in Chrome's DevTools. Not sure which WordPress-related colors we can use here? Ideas welcome!
There was a problem hiding this comment.
If we are looking for specific WordPress colors, perhaps the Color docs from the style guide might help. There's also a codepen with full list of official colors.
kevin940726
requested review from
ajitbohra,
mkaz,
ryanwelcher and
juanmaguitar
as code owners
|
+1 on the work here. Very impressive!
I tested out a mix of debounce and
However I guess we'd ideally want the visualizer to respond to every single value change. I'm not hugely experienced with the performance aspects of the observers. Do they play nicely with |
There was a problem hiding this comment.
This is impressive work and looks great so far @kevin940726! 👍
In testing this I only really hit two issues.
- As @apeatling mentioned, the re-rendering delay makes the experience feel pretty disjointed.
- The component will need to account for borders on its target.
| Left border only | Full Border |
|---|---|
 |
 |
I also have a few suggestions based on feedback I've received on recent component PRs that I'll list below but take them with a grain of salt.
It would be great to get some early feedback from @ciampo and @mirka regarding the latest approaches and best practices for the components package.
Some things we might wish to consider for this PR are:
- Restructuring the component directory and files.
- I was previously directed to the
ItemGroupcomponent as a great example of the latest approach for new components. - I followed this structure for the
ToolsPanel,BorderControl, andBorderBoxControlcomponents and it worked well for me.
- I was previously directed to the
- Related to the above, could we split the types out into their own
types.tsfile for consistency with other components and a cleaner component file? - Aiming to avoid all hardcoded classnames within new components and leverage dynamic classnames instead e.g. via
useCx.- Perhaps we could generate a dynamic classname and pass it to the Popover component and use that to hide the popover content.
- I believe there is a preference for using Emotion's
cssnow rather thanstyledcomponents
Again, this is great work. Looking forward to seeing improved visualizers throughout our blocks!
| // Copied from Chrome's DevTools: https://github.com/ChromeDevTools/devtools-frontend/blob/088a8f175bd58f2e0e2d492e991a3253124d7c11/front_end/core/common/Color.ts#L931 | ||
| const MARGIN_COLOR = 'rgba( 246, 178, 107, 0.66 )'; | ||
| // Copied from Chrome's DevTools: https://github.com/ChromeDevTools/devtools-frontend/blob/088a8f175bd58f2e0e2d492e991a3253124d7c11/front_end/core/common/Color.ts#L927 | ||
| const PADDING_COLOR = 'rgba( 147, 196, 125, 0.55 )'; |
There was a problem hiding this comment.
If we are looking for specific WordPress colors, perhaps the Color docs from the style guide might help. There's also a codepen with full list of official colors.
|
Thanks for the review! I think the problem is in We'll need to decide if this is that much of a problem right now. Would we allow users to resize their elements smoothly while also showing the overlay simultaneously? If there's a legit use case, we'll need to improve the performance for sure. But if there's not, then I'd say let's not prematurely optimize it? |
Ahh, right! I totally forgot about borders 😅. Will push up a fix soon. |
I'd say we're going to need to optimize this before it gets used in the editor. I think at this point you could simply confirm this direction isn't a dead end, and that kind of optimization is possible with |
I still think it depends on the usage. If the component is only going to be used when hovering over some The resizable box in the storybook is just a demonstration that it'll update itself when the size of the target changes. Maybe that's a bad example though and should probably just be a control field 😅. Maybe we can provide an opt-in prop like Currently, the issue only surfaces itself when the users are trying to edit the content of the target, specifically when it changes its size. Let's think about the purpose of this component: it shows an overlay to visualize the box model of the target element; why would the users want to have an overlay on top of the content they're trying to edit at the same time? Maybe there's a magical prop on |
There was a problem hiding this comment.
Hey @kevin940726 , thank you for working on this!
I gave the component a spin in Storybook, and noticed that the rendering of the overlay is a bit glitchy:
box-model-overlay.mp4
I'm going to echo @aaronrobertshaw 's comments:
- Restructuring the component directory and files.
- I was previously directed to the
ItemGroupcomponent as a great example of the latest approach for new components.- I followed this structure for the
ToolsPanel,BorderControl, andBorderBoxControlcomponents and it worked well for me.- Aiming to avoid all hardcoded classnames within new components and leverage dynamic classnames instead e.g. via
useCx.
- Perhaps we could generate a dynamic classname and pass it to the Popover component and use that to hide the popover content.
- I believe there is a preference for using Emotion's
cssnow rather thanstyledcomponents
I'm going to add that most of those aspects are addressed in the contributing guidelines.
Regarding the rendering performance of the component, I wonder if we could improve it by:
- debouncing the
updatefunction. Basically, while theupdatefunction is waiting to run and therefore while the overlay's values are out of sync, we could switch to a different UI (something very minimal that indicates this "waiting/recalculating" status) - re-write the
updatefunction so that it doesn't set styles and custom properties multiple times per call, which may cause multiple repaints / reflows in the browser. In that sense, re-writing those styles in Emotion (and batching their calculation "at once") may help a lot!
| '**/@(__mocks__|__tests__|test)/**/*.[tj]s?(x)', | ||
| '**/@(storybook|stories)/**/*.[tj]s?(x)', |
There was a problem hiding this comment.
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)
| 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.
(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?
There was a problem hiding this comment.
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.
Makes sense, let's address this in a follow-up PR
| }; | ||
| ``` | ||
|
|
||
| `<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.
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 ?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| }; | ||
| ``` | ||
|
|
||
| `<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.
Similarly to the previous comment, we usually don't want to explicitly explain how a component works internally
| `<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. |
There was a problem hiding this comment.
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.
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.
Fair point! Makes sense to me 👍 .
| // To ignore the error for incorrect types in BoxControl. | ||
| id={ undefined } | ||
| units={ undefined } | ||
| sides={ undefined } |
There was a problem hiding this comment.
Is this error something that should be fixed by marking these props as non-required in BoxControl ? Otherwise, I'd argue that we should try to use components as close as possible to how they're supposed to be used.
There was a problem hiding this comment.
Yep, the error is from <BoxControl>'s types.
There was a problem hiding this comment.
Gotcha. We can leave the code as-is for the purpose of this PR, but ideally we should follow-up in a new PR with those fixes
| // Copied from Chrome's DevTools: https://github.com/ChromeDevTools/devtools-frontend/blob/088a8f175bd58f2e0e2d492e991a3253124d7c11/front_end/core/common/Color.ts#L931 | ||
| const MARGIN_COLOR = 'rgba( 246, 178, 107, 0.66 )'; | ||
| // Copied from Chrome's DevTools: https://github.com/ChromeDevTools/devtools-frontend/blob/088a8f175bd58f2e0e2d492e991a3253124d7c11/front_end/core/common/Color.ts#L927 | ||
| const PADDING_COLOR = 'rgba( 147, 196, 125, 0.55 )'; | ||
|
|
||
| const OverlayPopover = styled( Popover )` | ||
| && { | ||
| pointer-events: none; | ||
| box-sizing: content-box; | ||
| border-style: solid; | ||
| border-color: ${ MARGIN_COLOR }; | ||
| // The overlay's top-left point is positioned at the center of the target, | ||
| // so we'll have add some negative offsets. | ||
| transform: translate( -50%, -50% ); | ||
|
|
||
| &::before { | ||
| content: ''; | ||
| display: block; | ||
| position: absolute; | ||
| box-sizing: border-box; | ||
| height: var( --wp-box-model-overlay-height ); | ||
| width: var( --wp-box-model-overlay-width ); | ||
| top: var( --wp-box-model-overlay-top ); | ||
| left: var( --wp-box-model-overlay-left ); | ||
| border-color: ${ PADDING_COLOR }; | ||
| border-style: solid; | ||
| border-width: var( --wp-box-model-overlay-padding-top ) | ||
| var( --wp-box-model-overlay-padding-right ) | ||
| var( --wp-box-model-overlay-padding-bottom ) | ||
| var( --wp-box-model-overlay-padding-left ); | ||
| } | ||
|
|
||
| .components-popover__content { | ||
| display: none; | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
These values should be moved to a separate styles.ts file and applied using Emotion
| mutationObserver.observe( target, { | ||
| attributes: true, | ||
| attributeFilter: [ 'style' ], | ||
| } ); |
There was a problem hiding this comment.
Would this be able to catch style changes caused from external stylesheets ?
There was a problem hiding this comment.
Nope. That would be some edge cases where the update method might be helpful.
There was a problem hiding this comment.
I see. Let's keep this approach for now and keep our eyes open to understand how impactful this ay be
| data-testid="box-model-overlay" | ||
| showValues={ DEFAULT_SHOW_VALUES } | ||
| > | ||
| <div data-testid="box" style={ { height: 300, width: 300 } } /> |
There was a problem hiding this comment.
Is there anything else we can use before resorting to data-testid ? Does the overlay have any role / accessible label or content that we could use instead?
This comment applies in general to all tests below using data-testid
There was a problem hiding this comment.
Nope, the overlay is supposed to be inaccessible to SR.
I don't think using data-testid here is that bad though. It's a way to target a specific element in tests, which is precisely what I need in these tests. Furthermore, they're not part of the component but just part of the tests, so we don't have to limit ourselves to using accessible queries IMO.
There was a problem hiding this comment.
Gotcha — in that case, let's leave these tests as they currently are.
| const box = screen.getByTestId( 'box' ); | ||
| const overlay = screen.getByTestId( 'box-model-overlay' ); | ||
|
|
||
| act( () => { | ||
| expect( targetRef.current ).toBe( box ); | ||
| } ); |
There was a problem hiding this comment.
Why don't we add some text in the "box" and then use screen.getByText to make sure it's rendered correctly? This way of testing is closer to how a user interacts with the component and expects it to work
There was a problem hiding this comment.
Using screen.getByText only makes sure the text exists but doesn't make sure targetRef.current is the target element. Both are valid though and the former could be used in other tests.
There was a problem hiding this comment.
Understood — we can leave things as they currently are.
| it( 'should correctly unmount the component', async () => { | ||
| const { unmount } = render( | ||
| <BoxModelOverlay | ||
| data-testid="box-model-overlay" | ||
| showValues={ DEFAULT_SHOW_VALUES } | ||
| > | ||
| <div data-testid="box" style={ { height: 300, width: 300 } } /> | ||
| </BoxModelOverlay> | ||
| ); | ||
|
|
||
| unmount(); | ||
| } ); |
There was a problem hiding this comment.
What are we testing for in particular in this test?
There was a problem hiding this comment.
Testing that it unmounts without any errors. It'd be better if there's an explicit assertion to make though.
There was a problem hiding this comment.
What in particular during the unmounting could go wrong? Is it the de-registration of the size / mutation observers? Maybe we could just add a quick code comment to explain why this test is required / important
There was a problem hiding this comment.
TBH, it's more like a sanity check 😅. I'm okay if we just remove this and add it back when there's a need.
There was a problem hiding this comment.
The main reason why I'm asking is because we don't usually test for component un-mounting, and so I was curious if there was a specific reason for it (and if we could learn something from your PR!).
I'm okay if we just remove this and add it back when there's a need.
Let's do this then, if you don't mind! Thank you :)
| ### `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 |
There was a problem hiding this comment.
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.
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.
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).
This looks like a bug in the
Oh nice! I didn't know that there's a separate contributing guideline specifically for the components package. I'll check it out! I have to admit that I don't agree with all of them though, but I guess it has already been discussed before? Is there any relevant discussion I can refer to? I'll still try to apply them in this PR though to make it consistent.
The problem isn't with debouncing or not. If any, the problem is actually caused by debouncing. Again, the rendering issue is caused by I'll echo my comment here, performance optimization isn't free. We can make some trade-offs, but I'll like to see some solid use cases before we do so. Or else we're just chasing an invisible target blindly.
Dynamic styles are not suitable to be generated in CSS-in-JS, each value will generate a unique hash, and AFAIK, emotion won't destroy these cache, which means there's going to be thousands of style tags in Using other techniques like overriding However, as I said before, I don't think the bottleneck is in that |
|
@kevin940726 Looking at the performance, it does look like The problem might be that we're passing a stable reference for The I wonder if it'd be better for |
There was a problem hiding this comment.
This looks like a bug in the
<BoxControl>component, whereonChangeShowVisualizeris behaving incorrectly when not hovering on the input.
Alright, let's address this in a follow-up PR.
I have to admit that I don't agree with all of them though, but I guess it has already been discussed before? Is there any relevant discussion I can refer to? I'll still try to apply them in this PR though to make it consistent.
These conventions were discussed and applied at the time they were introduced — before I started working on components. We've trying to enforce then as a way to have a bit more of a coherent style across components.
Feel free to create a new issue and list out what you disagree with - we're always open to this kind of feedback (although we may not currently have the capacity to carry out large changes!)
The problem isn't with debouncing or not. If any, the problem is actually caused by debouncing. Again, the rendering issue is caused by
<Popover>, which debounces the update by 500ms, so there's a delay when resizing the target element.I'll echo my comment here, performance optimization isn't free. We can make some trade-offs, but I'll like to see some solid use cases before we do so. Or else we're just chasing an invisible target blindly.
Thank you for the explanation! What about making the component transparent while saiting for the debounced update to take effect?
Dynamic styles are not suitable to be generated in CSS-in-JS, each value will generate a unique hash, and AFAIK, emotion won't destroy these cache, which means there's going to be thousands of style tags in
<head>😅.Using other techniques like overriding
cssTextdirectly could work though, but that means we'll have to create a wrapper for it so that it won't override the Popover's styles.However, as I said before, I don't think the bottleneck is in that
updatefunction. Updating padding's values is relatively smooth.
Gotcha — let's definitely not over-optimise the update function for now, especially if we think it's not the bottleneck.
Finally, I'm going to recap a few follow-up tasks that were discussed:
- add border support
- fix
BoxControltypes - fix glitchy behaviour of
onChangeShowVisualizerinBoxControl
| '**/@(__mocks__|__tests__|test)/**/*.[tj]s?(x)', | ||
| '**/@(storybook|stories)/**/*.[tj]s?(x)', |
There was a problem hiding this comment.
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
| 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.
Makes sense, let's address this in a follow-up PR
| }; | ||
| ``` | ||
|
|
||
| `<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.
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!
| }; | ||
| ``` | ||
|
|
||
| `<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.
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).
| mutationObserver.observe( target, { | ||
| attributes: true, | ||
| attributeFilter: [ 'style' ], | ||
| } ); |
There was a problem hiding this comment.
I see. Let's keep this approach for now and keep our eyes open to understand how impactful this ay be
| // To ignore the error for incorrect types in BoxControl. | ||
| id={ undefined } | ||
| units={ undefined } | ||
| sides={ undefined } |
There was a problem hiding this comment.
Gotcha. We can leave the code as-is for the purpose of this PR, but ideally we should follow-up in a new PR with those fixes
| data-testid="box-model-overlay" | ||
| showValues={ DEFAULT_SHOW_VALUES } | ||
| > | ||
| <div data-testid="box" style={ { height: 300, width: 300 } } /> |
There was a problem hiding this comment.
Gotcha — in that case, let's leave these tests as they currently are.
| const box = screen.getByTestId( 'box' ); | ||
| const overlay = screen.getByTestId( 'box-model-overlay' ); | ||
|
|
||
| act( () => { | ||
| expect( targetRef.current ).toBe( box ); | ||
| } ); |
There was a problem hiding this comment.
Understood — we can leave things as they currently are.
| it( 'should correctly unmount the component', async () => { | ||
| const { unmount } = render( | ||
| <BoxModelOverlay | ||
| data-testid="box-model-overlay" | ||
| showValues={ DEFAULT_SHOW_VALUES } | ||
| > | ||
| <div data-testid="box" style={ { height: 300, width: 300 } } /> | ||
| </BoxModelOverlay> | ||
| ); | ||
|
|
||
| unmount(); | ||
| } ); |
There was a problem hiding this comment.
What in particular during the unmounting could go wrong? Is it the de-registration of the size / mutation observers? Maybe we could just add a quick code comment to explain why this test is required / important
|
Just found this PR. I accidentally solved the same issue while working on something else #40505 |
I disagree though. IMO, using Furthermore, adding a new component here introduces the opportunity to write them in TypeScript, and make them testable in isolation, which I believe is the best practice forward? (At least it's what's suggested in the original issue 😅) This PR also takes a different approach by using |
|
The BlockPopover is a component that we already have and that have been used for several places "BlockToolbar", "InbetweenInserter"...
One of them is a temporary measure, because I didn't want to introduce a new dependency
Can we use this energy to type "Popover" instead of adding a new unused component (like we do for Flyout)
Sure, that's what useResizeAware or useResizeObserver is about :) |
My point is that it's often better to focus on one thing in a specific component. Instead of adding more responsibility to
The problem isn't only about using
Ideally, yes. In practice, though, It's not an unused component, we have plans for this component (described in the Next steps section in the PR description.) I just want to make this PR minimal and then we can work on integrating it into the existing codebase.
I don't see how #40505 suffers from this bug and will not work if the paddings have percentage values and the parent element has a different width. |
I agree with the point of having one responsibility but as I see it, this component being proposed has two: compute the position and size, render the visualizer. I'm fine with extracting a visualizer component but computing size and potentially (margin/padding) with a different approach (getComputedStyle) should be reusable hooks: BlockPopover internally uses It also avoids having a weird component API: either take a ref as prop which is never a good thing because we can't watch refs (a mistake we did in the early days of "Popover") or clone element to inject a ref. |
Making everything a hook feels over-engineered to me. The computing logic only exists because this component needs it, they have the same responsibility. Abstracting it away into a hook is premature optimization. Moreover, using hooks also means that we're going to leverage React's reconciler to update the styles, which is an unnecessary performance overhead for a pure styling component like this.
We can always extract common parts if we need to. But AFAIK,
These are strong words used here (weird, never), but I respectfully disagree. Using As for cloning elements, I believe some call this pattern "compound components". It's a pretty common pattern and it's used in In conclusion,
|
I disagree, hooks are way simpler primitives to share behavior than components. Components are meant to share logic that is rendering related (visualizer is a good example) but hooks are better to share "DOM related"/"state/effect related logic.
That's true for react-resize-aware but not useResizeObserver which is why I have this PR #40509
|
| } ); | ||
|
|
||
| // Percentage paddings are based on parent element's width, | ||
| // so we need to also listen to the parent's size changes. |
There was a problem hiding this comment.
This is interesting.
What about if padding and margin use VH,VW? These depend on the window sizes, are we subscribing to these changes as well.
Also for Rem unit, should we subscribe to the font size of the whole document?
I'd love if we can encapsulate that logic in a reusable hook const { ref, padding, margin } = usePaddingMarginObserver() it can also support a ref argument const { padding, margin } = usePaddingMarginObserver( ref )
WDYT?
There was a problem hiding this comment.
What about if padding and margin use VH,VW? These depend on the window sizes, are we subscribing to these changes as well.
These will have to be handled by the developers manually via the ref.current.update() function. I don't think this can ever be done perfectly without major performance penalties (repeatedly calling window.getComputedStyle() in requestAnimationFrame for instance). I'd suggest handling it for 80% or more of the use cases by default, and also providing an escape hatch like ref.current.update() to handle the rest.
I'd love if we can encapsulate that logic in a reusable hook
const { ref, padding, margin } = usePaddingMarginObserver()it can also support a ref argumentconst { padding, margin } = usePaddingMarginObserver( ref )
I don't think returning these styles is a good idea for this hook. The returned styles will have to be passed to the overlay's inline styles. This means whenever the observer observes the changes, the styles will be updated and the overlay component will have to be re-rendered. This could be costly and unnecessary, while we can update the style attribute on the DOM directly to achieve the same effects.
In case we want to update the design of the overlay to have more complex styles, we could render a canvas and draw on it instead. We can do all this inside the hook and encapsulate them all together inside a component.
Yes, the hook might be useful somewhere else, but let's not prematurely optimize it and assume that it'd be used by some other components. We can always extract the logic into a sharable hook if we need it. Doing it prematurely means we need to take care of wider API surfaces and be careful of the backward-compatibility issues. That's why I think making it a component is the right abstraction until proven otherwise.
This is just my opinion though, as long as you think it's easier to maintain, either way works fine with me.
What?
Address a part of #40057 by introducing a new component:
<BoxModelOverlay>.Why?
See #40057 for more info.
How?
The new component calculates the box model properties of the target element by using
window.getComputedStyleto correctly reflects its style. The component also uses<Popover>to position the overlay outside the target element's tree so that it won't interfere with the styles as mentioned in the original issue.Why using
window.getComputedStyle?Currently,
<BoxControl.Visualizer>accepts avaluesprop with the definedpaddingormarginto render the overlays. This is fundamentally flawed because we don't have enough context for the actual rendered style of the target. This is especially true when we're using percentage values inpaddingsince that padding percentage is based on the parent element's width.The only reliable way (AFAIK) to calculate the values is to use the browser's
getComputedStyleAPI. For that to work, we have to get a reference of the target element itself. This also means that we don't need to pass in thevaluesprop anymore, but we do need to listen to the style/size changes of the element to update the overlays.Why the name
BoxModelOverlay?BoxControl.Visualizeris more like a name that ties to the<BoxControl>component: it visualizes the box control's values; On the other hand,BoxModelOverlaymatches more closely to what the component does: it renders a box model overlay on top of the target element.Even though the name has "box model" in it, we could probably evolve when we decide to also visualize other properties like
flex,grid, orgap.Testing Instructions
Tests should pass.
npm run storybook:devScreenshots or screencast
Kapture.2022-04-12.at.21.03.26.mp4
Next steps
<BoxControl.Visualizer>to using<BoxModelOverlay>. (I think currently only Cover is using it?)<BoxModelOverlay>from being experimental and deprecate<BoxControl.Visualizer>.