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: Refactor Heading block to use class names for text align #16035

Merged
merged 5 commits into from Jun 14, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jun 7, 2019

Description

Related: #16027, #15751

This PR explores using className for the text alignment in Heading block. It also moves this alignment to the block toolbar. There is a deprecation strategy necessary to make it work. It has been discussed with @kjellr and @jasmussen in #15964 (comment). This should allow us to make it easier to customize more complex blocks like List or Quote where we modify multiple styles depending on the text alignment selected. This will also allow us to simplify styles used, e.g.:

&[style*="text-align:right"],
&[style*="text-align: right"] {
border-left: none;
border-right: 4px solid $black;
padding-left: 0;
padding-right: 1em;
}
&[style*="text-align:center"],
&[style*="text-align: center"] {
border: none;
padding-left: 0;
}

There are some important things to highlight:

  • align is used with other blocks for the block alignment, renaming this attribute to textAlign should better express its intent
  • in case of Heading using the class name shouldn't be an issue at all. I'm wondering if the same approach can be used for Quote block in subsequent PR
  • there are other blocks which could get the same update: Paragraph, Quote, and Verse

Before:

<!-- wp:core/heading {"align":"right","level":3} -->
<h3 style="text-align:right">A picture is worth a thousand words, or so the saying goes</h3>
<!-- /wp:core/heading -->

After:

<!-- wp:heading {"level":3,"textAlign":"right"} -->
<h3 class="has-text-align-right">A picture is worth a thousand words, or so the saying goes</h3>
<!-- /wp:heading -->

I also moved the control with text alignments to the block toolbar. This requires some tweaks for the level option which needs more love. I can remove it from this PR if it distracts the review process. I'm mostly concern about answering the question of whether we want to refactor the text alignment to work with class names. - removed from this PR. There are no visual changes.

How has this been tested?

Using one of the existing branches (master probably) add a few Heading blocks and set different text alignments and save your post. Open the same post with this branch and ensure that it still works

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 [Type] Enhancement A suggestion for improvement. [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Package] Block library /packages/block-library [Block] Heading Affects the Headings Block labels Jun 7, 2019
@gziolo gziolo self-assigned this Jun 7, 2019
@gziolo gziolo marked this pull request as ready for review June 7, 2019 10:58
@gziolo gziolo force-pushed the update/text-align-class-names branch from f87915e to dffab8b Compare June 10, 2019 11:22
@gziolo gziolo force-pushed the update/text-align-class-names branch 3 times, most recently from 04d9423 to 8a464f4 Compare June 10, 2019 15:06
@talldan talldan mentioned this pull request Jun 12, 2019
5 tasks
@timelsass
Copy link
Contributor

just leaving a note that I tested this out locally, and it resolved a few issues I had with block styles, good work! 👍

@gziolo gziolo requested a review from a team June 13, 2019 14:25
@gziolo
Copy link
Member Author

gziolo commented Jun 13, 2019

I also moved the control with text alignments to the block toolbar. This requires some tweaks for the level option which needs more love. I can remove it from this PR if it distracts the review process. I'm mostly concern about answering the question of whether we want to refactor the text alignment to work with class names.

I'm rather leaning towards moving it back to the sidebar to land the changes related to text alignment first and then replicate it for other core blocks in a follow-up PR.

@youknowriad
Copy link
Contributor

I'm rather leaning towards moving it back to the sidebar to land the changes related to text alignment first and then replicate it for other core blocks in a follow-up PR.

I think it makes sense. It would help for reviews.

@gziolo gziolo force-pushed the update/text-align-class-names branch from 8a464f4 to 2f0a325 Compare June 14, 2019 10:14
@gziolo
Copy link
Member Author

gziolo commented Jun 14, 2019

I'm rather leaning towards moving it back to the sidebar to land the changes related to text alignment first and then replicate it for other core blocks in a follow-up PR.

I think it makes sense. It would help for reviews.

Removed with 2f0a325.

@youknowriad
Copy link
Contributor

Is this a change that is going to be applied to all blocks? Thinking about consistency?

Also, if it's the case, are we considered about the deprecations needed (especially the paragraph block), this might create a big number of warnings :)

@gziolo
Copy link
Member Author

gziolo commented Jun 14, 2019

Is this a change that is going to be applied to all blocks? Thinking about consistency?

Yes, there are 3 more blocks to update:

  • Paragraph
  • Quote
  • Verse

We are going to add it to List block as well.

Also, if it's the case, are we considered about the deprecations needed (especially the paragraph block), this might create a big number of warnings :)

This will trigger in case non-default text alignment is applied. We should rather make those warning less scary when block's HTML get properly updated to use the latest version :)

@youknowriad
Copy link
Contributor

This will trigger in case non-default text alignment is applied. We should rather make those warning less scary when block's HTML get properly updated to use the latest version :)

Agreed, I think we can entirely avoid the warning and maybe just write a friendly notice.

@gziolo gziolo merged commit 93e31d0 into master Jun 14, 2019
@gziolo gziolo deleted the update/text-align-class-names branch June 14, 2019 10:59
@gziolo gziolo added this to the Gutenberg 6.0 milestone Jun 14, 2019
@gziolo gziolo removed the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Jun 14, 2019
daniloercoli added a commit that referenced this pull request Jun 14, 2019
…rnmobile/open-video-by-browser-for-android

* 'master' of https://github.com/WordPress/gutenberg: (34 commits)
  [RNMobile] Native mobile release v1.7.0 (#16156)
  Scripts: Document and simplify changing file entry and output of build scripts (#15982)
  Block library: Refactor Heading block to use class names for text align (#16035)
  Make calendar block resilient against editor module not being present (#16161)
  Bump plugin version to 5.9.1
  Editor: Fix the issue where statics for deprecated components were not hoisted (#16152)
  Build Tooling: Use "full" `npm install` for Build Artifacts Travis task (#16166)
  Scripts: Fix naming conventions for function containing CLI keyword (#16091)
  Add native support for Inserter (Ported from gutenberg-mobile) (#16114)
  docs(components/higher-order/with-spoken-messages): fix issue in example code (#16144)
  docs(components/with-focus-return): fix typo in README.md (#16143)
  docs(block-editor/components/inspector-controls): fix image path in README.md (#16145)
  Add mention of on Figma to CONTRIBUTING.md (#16140)
  Bring greater consistency to placeholder text for media blocks. (#16135)
  Add descriptive text and a link to embed documentation in embed blocks (#16101)
  Update toolbar-text.png (#16102)
  Updating changelogs for the Gutenberg 5.9 packages release
  chore(release): publish
  [RNMobile] Fix pasting text on Post Title (#16116)
  Bump plugin version to 5.9.0
  ...

# Conflicts:
#	packages/block-library/src/video/video-player.android.js
@gziolo gziolo added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jul 31, 2019
@gziolo
Copy link
Member Author

gziolo commented Jul 31, 2019

Applying Needs Dev Note as this might have an impact on themes which used the presence of style as a way to change the visual aspect of the block.

@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Heading Affects the Headings Block [Package] Block library /packages/block-library [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants