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

Buttons block: lighten editor DOM even more. #23222

Merged
merged 2 commits into from
Sep 25, 2020

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Jun 16, 2020

Description

This makes the editor DOM of the Buttons block identical to the front-end (in terms of div-wrapping) by removing several wrapper divs thanks to the __experimentalTagName property on <InnerBlocks />.

Note that this PR does not change the React Native edit implementation.

Before:
image

After:
image

How has this been tested?

I tested to make sure Buttons blocks still looked as expected in the editor, with or without a block alignment set.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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.

@ZebulanStanphill ZebulanStanphill added [Type] Code Quality Issues or PRs that relate to code quality [Package] Block library /packages/block-library [Block] Buttons Affects the Buttons Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. CSS Styling Related to editor and front end styles, CSS-specific issues. labels Jun 16, 2020
@github-actions
Copy link

github-actions bot commented Jun 16, 2020

Size Change: -6 B (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-library/editor-rtl.css 8.57 kB -4 B (0%)
build/block-library/editor.css 8.58 kB -4 B (0%)
build/block-library/index.js 135 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.52 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 8.53 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 128 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/style-rtl.css 7.61 kB 0 B
build/block-library/style.css 7.6 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.78 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.5 kB 0 B
build/components/index.js 167 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 9.42 kB 0 B
build/core-data/index.js 12 kB 0 B
build/data-controls/index.js 1.27 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.42 kB 0 B
build/edit-navigation/index.js 10.4 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.25 kB 0 B
build/edit-post/style.css 6.24 kB 0 B
build/edit-site/index.js 20.5 kB 0 B
build/edit-site/style-rtl.css 3.54 kB 0 B
build/edit-site/style.css 3.54 kB 0 B
build/edit-widgets/index.js 17.5 kB 0 B
build/edit-widgets/style-rtl.css 2.8 kB 0 B
build/edit-widgets/style.css 2.8 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.4 kB 0 B
build/editor/style-rtl.css 3.83 kB 0 B
build/editor/style.css 3.82 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.49 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.55 kB 0 B
build/is-shallow-equal/index.js 709 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.44 kB 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.7 kB 0 B
build/server-side-render/index.js 2.61 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.74 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ZebulanStanphill ZebulanStanphill added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Jun 16, 2020
@ZebulanStanphill ZebulanStanphill force-pushed the update/buttons-block-even-lighter-editor-dom branch from 73b3f42 to 7d7e3e4 Compare June 16, 2020 23:19
@ZebulanStanphill ZebulanStanphill marked this pull request as ready for review June 16, 2020 23:40
@oxyc
Copy link
Member

oxyc commented Jun 17, 2020

I'm seeing some broken styles with Twentytwenty.

First part is before this patch, notice the spacing betwen buttons (center aligned) and how the inner button inserter is hidden when out of focus. The second part I switch to an editor after this PR where the spacing is increased and the inserter never disappears.

Screen Recording 2020-06-16 at 21 08 30

@ZebulanStanphill ZebulanStanphill force-pushed the update/buttons-block-even-lighter-editor-dom branch from 7d7e3e4 to f0aa9c4 Compare June 17, 2020 00:48
@ZebulanStanphill
Copy link
Member Author

@oxyc Thanks for catching those. The margin issue should be fixed now.

The appender issue seems to be a lot more difficult to fix, however. I think the block wrapper is broken since AlignmentHookSettingsProvider is now wrapping the InnerBlocks (that is also the Block.div block wrapper), thus causing the block wrapper props to be applied to a div that isn't the root of the block. This seems to mess up the show/hide logic for the appender. Or at least that's my current hypothesis. I could be way off.

I'm not sure how to proceed here. The Buttons block is the only core block currently using AlignmentHookSettingsProvider. It uses it to disable the "block alignment" controls for nested Button blocks.

Pinging @jorgefilipecosta, since he introduced AlignmentHookSettingsProvider in #19824. Any suggestions for what to do here?

@ZebulanStanphill ZebulanStanphill added the Needs Technical Feedback Needs testing from a developer perspective. label Jun 17, 2020
@ZebulanStanphill ZebulanStanphill removed the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Jun 17, 2020
@ZebulanStanphill ZebulanStanphill force-pushed the update/buttons-block-even-lighter-editor-dom branch from f0aa9c4 to 2d58fdc Compare June 21, 2020 22:10
@youknowriad
Copy link
Contributor

youknowriad commented Jun 24, 2020

AlignmentHookSettingsProvider might be better as something internal to Innerblocks using a allowedAlignments prop or something like that. I'd love for this prop to be able to define the size of the alignments though and potentially custom alignments but that's a big challenge.

@ZebulanStanphill ZebulanStanphill force-pushed the update/buttons-block-even-lighter-editor-dom branch from 2d58fdc to 47a2c26 Compare June 24, 2020 17:03
@ZebulanStanphill ZebulanStanphill added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Jun 29, 2020
@ZebulanStanphill ZebulanStanphill force-pushed the update/buttons-block-even-lighter-editor-dom branch from 47a2c26 to ef59adb Compare July 1, 2020 00:50
@ZebulanStanphill ZebulanStanphill force-pushed the update/buttons-block-even-lighter-editor-dom branch from ef59adb to a851725 Compare August 30, 2020 11:47
@ZebulanStanphill
Copy link
Member Author

Thinking about it some more, I'm not actually sure if the appender not disappearing is a bug or not. The behavior in this PR is consistent with other blocks like the Group block, where the appender is always visible. #24836, if merged, will change this default behavior, and perhaps that will change the behavior for the Buttons block. I think I'll wait for that PR to be merged, and see what happens when I rebase this one.

@ZebulanStanphill
Copy link
Member Author

I looked into everything some more, and I discovered that the appender issue was actually unrelated to AlignmentHookSettingsProvider. The issue was actually #21805, which was actually a generic issue that can also be observed with the Social Icons block. The only difference between the two blocks was that the appender appears invisible for unselected Buttons blocks, but still takes up space, while it is simply always visible for the Social Icons block. The latter is technically "more expected" due to the current implementation. I'm not actually sure why the appender is invisible for the Buttons block in master.

Anyway, this PR actually fixed the weird invisible-appender bug, and in doing so, made the behavior consistent with the Social Icons block. But of course, the appender should simply not be there at all in the first place when these blocks (and their children) aren't selected. So I've created #25518 to fix this issue.

@ZebulanStanphill ZebulanStanphill force-pushed the update/buttons-block-even-lighter-editor-dom branch from de43266 to 78a08c4 Compare September 21, 2020 20:54
@ZebulanStanphill ZebulanStanphill removed the Needs Technical Feedback Needs testing from a developer perspective. label Sep 21, 2020
@ZebulanStanphill ZebulanStanphill force-pushed the update/buttons-block-even-lighter-editor-dom branch from 78a08c4 to 93bf03e Compare September 22, 2020 09:28
@ZebulanStanphill ZebulanStanphill force-pushed the update/buttons-block-even-lighter-editor-dom branch from 93bf03e to 0935149 Compare September 25, 2020 02:26
@ZebulanStanphill ZebulanStanphill removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Sep 25, 2020
@ZebulanStanphill
Copy link
Member Author

Alright, thanks to some fixes that landed in other PRs, this one was unblocked, and I've polished it up and rebased it. It is now ready for a final review.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM

width: auto;
.wp-block-buttons > .wp-block {
// Override editor auto block margins.
margin-left: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why this changed so much though?

Copy link
Member Author

Choose a reason for hiding this comment

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

I found that both the display and width properties were unnecessary. The former was already set by style.scss, and the latter was already auto by default. (And the editor never overrode it.) I tested with various alignments to make sure everything worked as expected.

I also found that the selector had a lot more specificity than it actually needed, so I simplified that. Notably, I chose to point at just the wp-block class for the children, to make it easier for 3rd-party button blocks to be inserted and styled correctly in the Buttons block. I almost just went with a .wp-block-buttons > * selector, but that turned out to not have enough specificity to override the margin-left value being set by the editor in some cases.

@ZebulanStanphill ZebulanStanphill merged commit a14ab7f into master Sep 25, 2020
@ZebulanStanphill ZebulanStanphill deleted the update/buttons-block-even-lighter-editor-dom branch September 25, 2020 14:44
@github-actions github-actions bot added this to the Gutenberg 9.1 milestone Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants