Dimensions panel: remove BoxControl Visualizer callbacks #36587
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…l />` temporarily until we can work out why triggering these callbacks onHover sets the post to dirty. As far I can see the Visualizer isn't used anywhere so I don't think there are any side-effects. Updating the README.md for `<BoxControl />` to communicate that `onChangeShowVisualizer` is not a required prop. The default is `noop`
ramonjd
added
[Type] Question
|
Size Change: -30 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
|
This fixed the problem for me and I didn't notice any negative effects in my testing - but will leave it to those that know a bit more about this part of the code to give a sign off on this approach |
The visualiser appears to be in use in the Cover block. At the moment, you can't see it immediately because the overlay background color seems to take precedence, but if you set a Cover block's overlay opacity down low, and then slide the padding control, you should see the visualizer. I imagine a separate follow-up would be to change the stacking order of the visualizer there to make sure it's visible (assuming we want to keep it?). I'm wondering if the reason that the So, the effect this has, is that the post editor treats this just as if we'd altered a "real" attribute that affects the block's state, like updating any other attribute. The behaviour of storing whether or not the visualiser is to be shown, within the block's attributes, doesn't quite feel like the right way for the visualizer to be implemented. Ideally, the editor UI state should be separate from what we're storing in the block's attributes, but I can't immediately think of an easy alternative in the short-term, to maintain the visualizer functionality. Considering that it isn't working properly for the Cover block anyway, I'm in support of the approach in this PR of removing it altogether for the BoxControls in padding and margin in the short-term. Curious to hear what other folks think, too! CC: @aaronrobertshaw |
I would agree with that, this doesn't seem to be a way that block attributes are intended to be used - but can't immediately think of an alternative either. I think the Cover block issue may have been caused by that change we made to allow the overlay opacity to be set for just a background color, so it may not be a case of it never worked so let's remove it. Changing the z-index of that div to 2 fixes it - but if we do that to fix that we need an alternative to this PR to fix the post dirty issue? |
|
I agree the post appears to be being set to dirty due to the use of An alternative approach could be using component state instead of the attributes. @ajlende already has a PR up taking this sort of approach to address the second issue ( #33560 ), which I believe might also solve the first problem here. There were also plans to overhaul the approach to visualizers in general and potentially render them as popovers. That plan might mean it's more acceptable to remove the visualizer in the short term. |
|
Thanks for chiming in, folks, and exploring the cause further. 🙇 That the To me, the dirty post side effect of hovering is an anomaly, and seems a little buggy. As others have mentioned, I don't think removing the callbacks temporarily while a solution works its way through the pipeline, would cause too much calamity. That's just my 2c. The Github issue however, has been given low priority, so if people agree that we can live with the effect while the underlying cause is addressed then we can close this PR and concentrate on the WIP PRs cited above. |
|
Given the importance placed upon getting visualizers right eventually for UX purposes and the low priority for the known issues. I'd vote for closing this now and putting our efforts towards the long term goal. |
ramonjd
deleted the
remove/dimensions-panel-box-control-visualizer-callback
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Maybe, possibly resolves #36418
Description
Here we're removing
onChangeShowVisualizercallbacks passed down to<BoxControl />for margin and padding block support controls temporarily until we can work out why triggering these callbacks onHover sets the post to dirty.See how the "Update" button is activated when we hover over a
<BoxControl />, in this case the padding control in the dimensions panel.Nov-18-2021.09-05-55.mp4
Ideally we'd track down where/how the post is being made dirty.
My suspicion, after playing with the component in Storybook, is that it's due to the dynamic CSS class updates that occur on the
<Visualizer />componentonHover.As far I can see the Visualizer isn't yet used anywhere in accordance with the dimensions panel, so I don't think there are any side-effects. I'm not sure.
Furthermore, attempts to add a Box Control
<Visualizer />to a couple of blocks in order to test out the functionality crashed my editor. 🤷Quite possibly I'm doing things wrong, so maybe we can dump this PR altogether if folks can think of a better way.
We're also updating the README.md for
<BoxControl />to communicate thatonChangeShowVisualizeris not a required prop. The default isnoopHow has this been tested?
Checklist:
*.native.jsfiles for terms that need renaming or removal).