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

Full Site Editing: Hovering block padding and margin inputs are registered as changes #36418

Closed
zaguiini opened this issue Nov 11, 2021 · 9 comments · Fixed by #40505
Closed
Assignees
Labels
[Feature] Block settings menu The block settings screen [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@zaguiini
Copy link
Member

zaguiini commented Nov 11, 2021

Description

Putting the mouse cursor on top of some block attributes inputs (e.g. padding, margin) enables the "Save" button on the top right, even though nothing changed.

This behavior exists because of the cover block padding and margin visualizer, introduced on #23041.

Note that we do want to keep the visualizer showing up when hovering over the input. What we don't want is to enable the "Save" button when this happens.

A possible solution would be to introduce a new piece of state that would host the visualizer settings itself, not polluting the changes object and not triggering the false alarm.

Step-by-step reproduction instructions

  1. Enable the full site editor locally;
  2. Navigate to full site editor;
  3. Select any block that has spatial settings and hover the padding or margin inputs;
  4. Check that the "Save" button is enabled.

Screenshots, screen recording, code snippet

Screen.Recording.2021-11-11.at.17.07.48.mov

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@jeyip jeyip added [Type] Bug An existing feature does not function as intended [Feature] Full Site Editing [Feature] Block settings menu The block settings screen labels Nov 11, 2021
@creativecoder
Copy link
Contributor

FYI @apeatling and friends. I think you may have been working on these kind of controls, recently.

@jeyip jeyip added the [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later label Nov 16, 2021
@ramonjd
Copy link
Member

ramonjd commented Nov 17, 2021

I can confirm this happens in the Post Editor as well

Nov-18-2021.09-05-55.mp4

@ramonjd
Copy link
Member

ramonjd commented Nov 17, 2021

I think it's related to the hover callbacks on <BoxControl />.

			onHoverOn={ handleOnHoverOn }
			onHoverOff={ handleOnHoverOff }

After commenting them out I can no longer reproduce the issue.

You can see the effect for margin and padding support controls, but not spacing, which uses <UnitControl />

Edit: The hover callbacks trigger the onChangeShowVisualizer prop. If we don't pass down this prop to <BoxControl /> the effect also doesn't materialize.

@ramonjd
Copy link
Member

ramonjd commented Nov 17, 2021

The hover callbacks trigger the onChangeShowVisualizer prop. If we don't pass down this prop to the effect also doesn't materialize.

I've thrown up a PR just in case doing this is okay (for now). I can't see any side effects so far – I see no usage of Visualizer, but I'm not an expert. cc @aaronrobertshaw for advice.

@aaronrobertshaw
Copy link
Contributor

Other than the spacing block supports, the Cover block also utilizes the box control's Visualizer.

I believe other blocks have already had their visualizers removed in favour of waiting for the general approach to visualizers to be updated. The plan there was to explore using popovers or something along those lines as visualization of margins is problematic at the moment and generally forced a disjoint between the markup in the editor vs frontend.

It's also been highlighted that in the current state of the Cover block, the visualizer is hidden underneath any applied overlay. With all that said, I wouldn't be against temporarily removing all visualizers. Others with a clearer view of the bigger picture might see it differently though.

@youknowriad do you have any thoughts on this? You've advised me previously on the plans for visualizers.

An alternative to removal might be to pursue the approach taken in #33560.

@youknowriad
Copy link
Contributor

I think the main issue here is that the visualizer relies on block attributes to keep its state which should be just local state. I believe @mcsf also had identified this and may have had solutions proposed somewhere.

@aaronrobertshaw
Copy link
Contributor

I think the main issue here is that the visualizer relies on block attributes to keep its state which should be just local state. I believe @mcsf also had identified this and may have had solutions proposed somewhere.

Thanks, agreed. I should have added further detail to my comment, sorry.

The use of block attributes has been discussed as the cause of this issue on @ramonjd's PR: #36587 (comment). It also causes a separate issue (#31839) where that visualizer data is persisted within the block attributes on save.

The alternative approach taken in #33560 attempts to move this visualizer data into component state. It might require some additional tweaks though. So I think the question is, should we push ahead with @ajlende's efforts in #33560 or remove the visualizers given their very limited and slightly buggy use?

I believe @mcsf also had identified this and may have had solutions proposed somewhere.

I took a look around open PRs but didn't spot anything. I'm happy to help out with whichever approach we decide on.

@youknowriad
Copy link
Contributor

For me visualizers are very important in terms of UX and we should look at ways to increase their usage in our code base.

The reason they're not used more extensively is because the way they're implemented, they forces you to have extra DOM wrappers in a lot blocks if you want to add them. I'd love personally if we solve this and increase their usage either by using a "Popover" to render them or by using :before or :after styles or any technique that don't mess too much with the DOM tree.

@ramonjd
Copy link
Member

ramonjd commented Nov 18, 2021

So I think the question is, should we push ahead with @ajlende's efforts in #33560 or remove the visualizers given their very limited and slightly buggy use?

I'd lean towards flipping them to "off" until we get #33560 or whichever other solution across the line.

It's a small and inexpensive change to remove incongruent behaviour in the editor.

Moreover, if folks start using the Visualizer in its current state (beyond the current usage in Cover), then it might make implementing a potential, future solution more difficult.

For me visualizers are very important in terms of UX and we should look at ways to increase their usage in our code base.

💯

We could bump the priority of this work, review #33560 and so on... What do folks think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block settings menu The block settings screen [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
6 participants