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

'align' support flag - add server side block support for block alignment. #24122

Merged
merged 4 commits into from
Jul 27, 2020

Conversation

Addison-Stavlo
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo commented Jul 22, 2020

Description

Expanding on #23007 - Updates the gutenberg_experimental_apply_classnames_and_styles function in lib/blocks.php to apply block alignment classnames for blocks that support the align flag and depend on a render callback.

This also adds a wrapper to the Template Part block on the front end. This block was previously given the align support flag to have block alignment work in the editor, but was never updated for the front-end. Since there was no wrapper for this block on the front-end, the classes were being added to the first child of the template part block instead and causing other issues with blocks nesting under that first child.

How has this been tested?

Tested on local docker environment.

This can be tested with the template part block:

  • Visit the Site Editor, set the header template part to 'full width' alignment. (This may already be the case for some themes such as seedlet-blocks).
  • Save this setting.
  • Visit the front-end. Inspect the header template part and verify that the alignfull class is added to the wrapper div.
    Screen Shot 2020-07-22 at 1 15 45 PM

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

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.

@github-actions
Copy link

github-actions bot commented Jul 22, 2020

Size Change: -16 B (0%)

Total Size: 1.15 MB

Filename Size Change
build/block-editor/index.min.js 125 kB -16 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.min.js 1.14 kB 0 B
build/annotations/index.min.js 3.67 kB 0 B
build/api-fetch/index.min.js 3.43 kB 0 B
build/autop/index.min.js 2.82 kB 0 B
build/blob/index.min.js 620 B 0 B
build/block-directory/index.min.js 7.63 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.61 kB 0 B
build/block-library/editor.css 7.61 kB 0 B
build/block-library/index.min.js 132 kB 0 B
build/block-library/style-rtl.css 7.82 kB 0 B
build/block-library/style.css 7.83 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.min.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.min.js 3.1 kB 0 B
build/blocks/index.min.js 48.3 kB 0 B
build/components/index.min.js 200 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.6 kB 0 B
build/compose/index.min.js 9.68 kB 0 B
build/core-data/index.min.js 11.5 kB 0 B
build/data-controls/index.min.js 1.29 kB 0 B
build/data/index.min.js 8.45 kB 0 B
build/date/index.min.js 5.38 kB 0 B
build/deprecated/index.min.js 772 B 0 B
build/dom-ready/index.min.js 568 B 0 B
build/dom/index.min.js 3.23 kB 0 B
build/edit-navigation/index.min.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.min.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/index.min.js 16.9 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.min.js 9.37 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.min.js 45.3 kB 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.min.js 4.65 kB 0 B
build/escape-html/index.min.js 733 B 0 B
build/format-library/index.min.js 7.72 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.min.js 2.13 kB 0 B
build/html-entities/index.min.js 621 B 0 B
build/i18n/index.min.js 3.56 kB 0 B
build/is-shallow-equal/index.min.js 711 B 0 B
build/keyboard-shortcuts/index.min.js 2.51 kB 0 B
build/keycodes/index.min.js 1.94 kB 0 B
build/list-reusable-blocks/index.min.js 3.12 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.min.js 5.32 kB 0 B
build/notices/index.min.js 1.79 kB 0 B
build/nux/index.min.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.min.js 2.56 kB 0 B
build/primitives/index.min.js 1.4 kB 0 B
build/priority-queue/index.min.js 789 B 0 B
build/redux-routine/index.min.js 2.85 kB 0 B
build/rich-text/index.min.js 13.9 kB 0 B
build/server-side-render/index.min.js 2.71 kB 0 B
build/shortcode/index.min.js 1.7 kB 0 B
build/token-list/index.min.js 1.27 kB 0 B
build/url/index.min.js 4.06 kB 0 B
build/viewport/index.min.js 1.85 kB 0 B
build/warning/index.min.js 1.14 kB 0 B
build/wordcount/index.min.js 1.17 kB 0 B

compressed-size-action

@Addison-Stavlo Addison-Stavlo changed the title WIP - add block alignment classes on server side style function 'align' support flag - add server side block support. Jul 22, 2020
@Addison-Stavlo Addison-Stavlo self-assigned this Jul 22, 2020
@Addison-Stavlo Addison-Stavlo marked this pull request as ready for review July 22, 2020 17:24
@Addison-Stavlo Addison-Stavlo changed the title 'align' support flag - add server side block support. 'align' support flag - add server side block support for block alignment. Jul 22, 2020
@ZebulanStanphill
Copy link
Member

Is conditionally wrapping the template part the right way to handle alignments? I can't think of a better way, but I'm just making sure, since I can't recall if any solid direction has been chosen yet as a result of the discussion in #20650.

Copy link
Contributor

@apeatling apeatling left a comment

Choose a reason for hiding this comment

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

Tested and confirmed that I see alignfull rendered in the front end. 👍 I think there will need to be the wrapper to apply top level classnames like this, otherwise there is nowhere for them to go.

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Jul 22, 2020

Is conditionally wrapping the template part the right way to handle alignments?

I feel like it needs a wrapper regardless. In the future this wrapper should be a semantic element specified by an attribute saved for that template part block (instead of div wrapper it should be header,footer,nav,section, etc.).

Stepping back to talk about alignments specifically - this PR is specific to the align hook. As noted in the initial post of #20650 the align hook uses this convention. This hook is set up to save the class name this way for blocks that have a save.js file, but blocks that require index.php currently have no support. Here, we are just adding support for the align hook for blocks that must use the index.php, and thus using the same convention in applying the class to the wrapper. The Template Part block itself has had the align hook supported in its block.json, but since it uses index.php these alignments were only supported in the editor and never added on the front-end.

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Jul 22, 2020

I think there will need to be the wrapper to apply top level classnames like this, otherwise there is nowhere for them to go.

Or worse, without the wrapper on the Template Part the classname was being added to the first interior block of the template part instead. This was also causing some odd nesting within that first interior block and rendering different sections of the page on top of each other.

@ZebulanStanphill ZebulanStanphill added the [Type] Bug An existing feature does not function as intended label Jul 22, 2020
@Addison-Stavlo
Copy link
Contributor Author

Im not sure why the Bug label was added? This isn't addressing a 'bug' but adding feature support that was never implemented for these type of blocks by expanding on #23007 as recommended for future improvement to this function #23007 (comment).

@ZebulanStanphill
Copy link
Member

I figured it was a bug fix PR since the alignment controls were already being shown but not actually doing anything. I guess it's sort of both an enhancement and a bug fix?

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Jul 23, 2020

I figured it was a bug fix PR since the alignment controls were already being shown but not actually doing anything. I guess it's sort of both an enhancement and a bug fix?

Ah gotcha, that makes sense. TBH, I think most of the blocks that this will end up supporting don't have the alignment flag added yet.

Looking further it seems both the "Template Part" and "Post Content" blocks were given the flag back when wide alignment support was added to the site editor #23488, but since that was all tested regarding wide support inside the site editor itself we must have missed that it wasn't being added on the front-end (although its expected that it wouldn't be since both of those blocks require the render callback).

@Addison-Stavlo
Copy link
Contributor Author

@youknowriad - would you mind giving this a glance here when you get a chance? Re expanding on #23007 to add support for the align hook. It seems good from our end, but it would be nice to have a sanity check here since this effects more than the experiment we are generally focused on. 😆

@youknowriad
Copy link
Contributor

It seems good at a glance. I didn't check but I think there's probably a bunch of existing dynamic blocks where you could remove code from (align applied ad-hoc ).

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Jul 27, 2020

I didn't check but I think there's probably a bunch of existing dynamic blocks where you could remove code from (align applied ad-hoc ).

It looks like the following blocks fit this case:

  • archives
  • calendar
  • categories
  • latest-comments
  • latest-posts
  • navigation
  • rss
  • search
  • tag-cloud

I will create a follow up to remove the unnecessary code for these. (created #24223)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants