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

ToolsPanelItem: Add panelId check before calling toggle methods #35375

Merged

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented Oct 6, 2021

Description

While testing a couple of different blockGap PRs (#34630 and #34546), I noticed that if I select a block and update its block spacing value, then select the same type of block and then back to the original block, the value I set would be cleared out. After adding some console.log()s, it appears that the onSelect and onDeselect functions in the Tools Panel Item were being called (effectively as though we'd toggled the blockGap control).

This PR adds a check before calling the onSelect and onDeselect functions to ensure that the current panel id hasn't changed before calling the functions. This appears to fix the issue of the blockGap value unexpectedly being cleared.

How has this been tested?

Manually in the editor.

Before testing, switch to an FSE-capable theme (e.g. TT1-Blocks) and enable the blockGap feature by updating the settings.spacing.blockGap setting in the theme.json file. For example, here's how that section of my theme.json file looks:

		"spacing": {
			"blockGap": true,
			"customPadding": true,
			"customMargin": true,
			"units": [
				"px",
				"em",
				"rem",
				"vh",
				"vw"
			]
		},

To replicate, we need to click directly between two blocks with the same dimensions panel, so one way to do this is to add two columns blocks to your post content, and set them to use a background color so that you can click between them. To make it easier for testing, here's a gist of post content with two columns blocks. Copy and paste it in the code editor view for testing.

Before applying this PR:

  1. Select the first columns block and set a value in the block spacing field
  2. Directly select the other columns block
  3. Click back to the first columns block and notice that the value you previously entered has been removed

It may take clicking back and forth a couple of times before you see the issue.

With this PR applied, the values should not be unexpectedly removed. Also test to make sure that toggling tools panel items in the ToolsPanel menu still works as expected.

Screenshots

Note that in the Before screengrab the block spacing value is cleared unexpectedly when clicking between Columns blocks. In the After screengrab the value is not cleared.

Before After
blockGap-before-sml blockGap-after-sml

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested. (manually)
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@andrewserong andrewserong added [Type] Bug An existing feature does not function as intended [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Oct 6, 2021
@andrewserong andrewserong self-assigned this Oct 6, 2021
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this one up and the detailed PR description @andrewserong 🙇

I could replicate the issue as described via clicking directly between the two columns blocks. It was a little surprising that switching between the blocks via the editor toolbar's List View didn't exhibit the same behaviour.

After applying this PR's changes, the value was correctly maintained.

LGTM 👍

@andrewserong andrewserong merged commit cdc5c15 into trunk Oct 6, 2021
@andrewserong andrewserong deleted the update/tools-panel-item-to-check-panel-id-before-toggle branch October 6, 2021 05:30
@github-actions github-actions bot added this to the Gutenberg 11.7 milestone Oct 6, 2021
@andrewserong andrewserong mentioned this pull request Oct 6, 2021
3 tasks
@ciampo ciampo mentioned this pull request Oct 7, 2021
62 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants