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

Block library: Standardize align and className attributes for dynamic blocks #14533

Merged
merged 9 commits into from
Mar 26, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Mar 20, 2019

Description

The current alignment of blocks:

  1. Archives: left, center, right
  2. (Reusable) Block: none (also no default class name - wp-block-*)
  3. Calendar: left, center, right, wide, full
  4. Categories: left, center, right
  5. Latest Categories: left, center, right, wide, full
  6. Latest Posts: left, center, right, wide, full
  7. Legacy Widget: none
  8. RSS: none
  9. Search: none
  10. Shortcode: none (also no default and custom class name)
  11. Tag Cloud: left, center, right, wide, full

How has this been tested?

Screenshots

Types of changes

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.

@gziolo gziolo added [Feature] Blocks Overall functionality of blocks [Feature] Block Directory Related to the Block Directory, a repository of block plugins [Status] In Progress Tracking issues with work in progress labels Mar 20, 2019
@mapk
Copy link
Contributor

mapk commented Mar 20, 2019

These are just the dynamic blocks, right? I think standardizing these alignments is a great idea. I'm a big fan of allowing any content to be aligned any way. Here's my proposal, but I could be convinced otherwise.

  1. Archives: add wide, full
  2. (Reusable) Block: Does a reusable block keep the full width if it was built that way from the beginning?
  3. Calendar: no change
  4. Categories: add wide, full
  5. Latest Categories: no change
  6. Latest Posts: no change
  7. Legacy Widget: Not sure, maybe just leave without alignment options?
  8. RSS: add all alignments
  9. Search: add all alignments
  10. Shortcode: add all alignments
  11. Tag Cloud: no change

@gziolo
Copy link
Member Author

gziolo commented Mar 21, 2019

Test post:

<!-- wp:paragraph {"textColor":"white","backgroundColor":"light-gray","fontSize":"small"} -->
<p class="has-text-color has-background has-small-font-size has-white-color has-light-gray-background-color">Archives</p>
<!-- /wp:paragraph -->

<!-- wp:archives {"align":"full"} /-->

<!-- wp:paragraph {"textColor":"white","backgroundColor":"light-gray","fontSize":"small"} -->
<p class="has-text-color has-background has-small-font-size has-white-color has-light-gray-background-color">Calendar</p>
<!-- /wp:paragraph -->

<!-- wp:calendar {"align":"full"} /-->

<!-- wp:paragraph {"textColor":"white","backgroundColor":"light-gray","fontSize":"small"} -->
<p class="has-text-color has-background has-small-font-size has-white-color has-light-gray-background-color">Categories</p>
<!-- /wp:paragraph -->

<!-- wp:categories {"align":"full"} /-->

<!-- wp:paragraph {"textColor":"white","backgroundColor":"light-gray","fontSize":"small"} -->
<p class="has-text-color has-background has-small-font-size has-white-color has-light-gray-background-color">Latest Comments</p>
<!-- /wp:paragraph -->

<!-- wp:latest-comments {"align":"full"} /-->

<!-- wp:paragraph {"textColor":"white","backgroundColor":"light-gray","fontSize":"small"} -->
<p class="has-text-color has-background has-small-font-size has-white-color has-light-gray-background-color">Latest Posts</p>
<!-- /wp:paragraph -->

<!-- wp:latest-posts {"align":"full"} /-->

<!-- wp:paragraph {"textColor":"white","backgroundColor":"light-gray","fontSize":"small"} -->
<p class="has-text-color has-background has-small-font-size has-white-color has-light-gray-background-color">Legacy Widget</p>
<!-- /wp:paragraph -->

<!-- wp:legacy-widget /-->

<!-- wp:paragraph {"textColor":"white","backgroundColor":"light-gray","fontSize":"small"} -->
<p class="has-text-color has-background has-small-font-size has-white-color has-light-gray-background-color">RSS</p>
<!-- /wp:paragraph -->

<!-- wp:rss {"align":"full","feedURL":"https://gziolo.pl"} /-->

<!-- wp:paragraph {"textColor":"white","backgroundColor":"light-gray","fontSize":"small"} -->
<p class="has-text-color has-background has-small-font-size has-white-color has-light-gray-background-color">Search</p>
<!-- /wp:paragraph -->

<!-- wp:search {"align":"full"} /-->

<!-- wp:paragraph {"textColor":"white","backgroundColor":"light-gray","fontSize":"small"} -->
<p class="has-text-color has-background has-small-font-size has-white-color has-light-gray-background-color">Shortcode</p>
<!-- /wp:paragraph -->

<!-- wp:shortcode /-->

<!-- wp:paragraph {"textColor":"white","backgroundColor":"light-gray","fontSize":"small"} -->
<p class="has-text-color has-background has-small-font-size has-white-color has-light-gray-background-color">Tag Cloud</p>
<!-- /wp:paragraph -->

<!-- wp:tag-cloud {"align":"full"} /-->

In the editor:

Screen Shot 2019-03-21 at 13 33 51

Screen Shot 2019-03-21 at 13 34 31

Screen Shot 2019-03-21 at 13 34 50

On the frontend:

Screen Shot 2019-03-21 at 13 35 37
Screen Shot 2019-03-21 at 13 36 22

@gziolo gziolo force-pushed the update/block-align-refactor branch from cc0e234 to d33b656 Compare March 21, 2019 12:37
@gziolo gziolo self-assigned this Mar 21, 2019
@gziolo
Copy link
Member Author

gziolo commented Mar 21, 2019

These are just the dynamic blocks, right? I think standardizing these alignments is a great idea. I'm a big fan of allowing any content to be aligned any way. Here's my proposal, but I could be convinced otherwise.

Yes, all of them have some rendering logic on the server. In your list I had only one concern:

Shortcode: add all alignments

I don't think it's technically possible as shortcodes are special. It's something lik [gallery id="1,2,3"] so we can't easily inject align there. I would skip it.

Legacy Widget: Not sure, maybe just leave without alignment options?

Agreed, both Shortcode and Legacy Widget exist for backward compatibility. We should leave them as they are.

When testing changes applied, I noticed that those align options need some CSS on the frontend. I'm surprised we don't have a global handler which would make it simpler. It also looks like some padding is necessary to make full-width alignment look good for all updated blocks. I shared HTML code in the previous comment so you could play with it yourself and advice how to approach it. Feel free to apply changes yourself as well 😺

@mapk
Copy link
Contributor

mapk commented Mar 23, 2019

When testing changes applied, I noticed that those align options need some CSS on the frontend.

Yes, it appears the RSS & Search blocks don't get the alignfull selector appended to them even though I've selected them to be alignfull in the editor. Notice in the Inspector screenshot below, the Latest Posts block has the alignfull selector, but the two below it don't.

inspector

I'm working on this right now.

Copy link
Contributor

@mapk mapk left a comment

Choose a reason for hiding this comment

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

Those last commits should fix the two blocks that wouldn't go full width. Now they all do. It doesn't look great on the frontend, but that's really where the themes come in, right?

If everything looks good, LGTM!

@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Mar 25, 2019
@gziolo gziolo force-pushed the update/block-align-refactor branch from 372b389 to 7da6f91 Compare March 25, 2019 08:38
@gziolo gziolo marked this pull request as ready for review March 25, 2019 08:38
@gziolo
Copy link
Member Author

gziolo commented Mar 25, 2019

If everything looks good, LGTM!

Yes, code changes look great, thanks for your help. In retrospect, I might at least tweak PHP part myself :)

@gziolo
Copy link
Member Author

gziolo commented Mar 25, 2019

There is one failing test after I rebased this PR with master branch:

FAIL specs/block-transforms.test.js (29.582s)
  ● Block transforms › should contain the expected transforms
    expect(jest.fn()).not.toHaveErrored(expected)
    Expected mock function not to be called but it was called with:
    ["Block validation: Block validation failed for `%s` (%o).
    Content generated by `save` function:
    %s
    Content retrieved from post body:
    %s core/shortcode JSHandle@object  [gallery ids=\"238,338\"]"]
      at expect (../jest-console/build/@wordpress/jest-console/src/index.js:34:4)

It looks like we need to land first PR which properly loads PHP files of blocks which are defined in WordPress core: #13521.

@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label Mar 25, 2019
@gziolo
Copy link
Member Author

gziolo commented Mar 26, 2019

Okey, I rebased with master branch and retested. I will merge if Travis is happy.

@gziolo gziolo force-pushed the update/block-align-refactor branch from 4f76957 to fe1af92 Compare March 26, 2019 15:20
@gziolo gziolo merged commit 8243a1c into master Mar 26, 2019
@gziolo gziolo deleted the update/block-align-refactor branch March 26, 2019 15:34
@youknowriad youknowriad added this to the 5.4 (Gutenberg) milestone Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Directory Related to the Block Directory, a repository of block plugins [Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants