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 Paragraph block to use class name for text align #16794

Merged
merged 4 commits into from Aug 8, 2019

Conversation

@gziolo
Copy link
Member

commented Jul 29, 2019

Description

Related: #16027, #15751, #16777, and #16779.

This PR follows-up #16035.

Before:

<!-- wp:core/paragraph {"align":"right"} -->
<p style="text-align:right;">... like this one, which is separate from the above and right aligned.</p>
<!-- /wp:core/paragraph -->

After:

<!-- wp:paragraph {"align":"right"} -->
<p class="has-text-align-right">... like this one, which is separate from the above and right aligned.</p>
<!-- /wp:paragraph -->

How has this been tested?

Using one of the existing branches (master probably) add a few Paragraph 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

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

When I go to Gutenberg demo page locally I see lots of warnings on JS console. It looks like validation runs twice, but more importantly in this case I need to update HTML source of the demo content. I plan to open a related PR where I will improve the feedback shared on the JS console. It clearly is wrong as is, we should only let developers know that post was properly converted and show what was before and how it is coded from now on.

Screen Shot 2019-07-29 at 17 00 52

@youknowriad, @aduth, @mtias or @mcsf, do you know why validation is triggered twice?

@aduth

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@gziolo If I had to guess without looking, we have a few issues present in development environments due to how React fires constructor twice in strict mode:

Strict mode can’t automatically detect side effects for you, but it can help you spot them by making them a little more deterministic. This is done by intentionally double-invoking the following methods:

  • Class component constructor method
  • [...]

https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects

For us, likely most problematic here:

props.setupEditor( props.post, props.initialEdits, props.settings.template );

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Validation also runs for every deprecated version until it finds the "correct" version to use. I think the path forward would be to not show any error unless there's no corresponding deprecated version.

@gziolo

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

@gziolo If I had to guess without looking, we have a few issues present in development environments due to how React fires constructor twice in strict mode:

Strict mode can’t automatically detect side effects for you, but it can help you spot them by making them a little more deterministic. This is done by intentionally double-invoking the following methods:

  • Class component constructor method
  • [...]

https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects

For us, likely most problematic here:

props.setupEditor( props.post, props.initialEdits, props.settings.template );

@aduth, you are totally correct. I tried with SCRIPT_DEBUG both enabled and disabled. I can see it triggered twice only in development mode which confirms what you shared. I guess we can ignore it then or refactor this provider to use hooks.

Validation also runs for every deprecated version until it finds the "correct" version to use. I think the path forward would be to not show any error unless there's no corresponding deprecated version.

@youknowriad – I'm working on it in separate PR 👍

@gziolo

This comment has been minimized.

Copy link
Member Author

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.

@gziolo gziolo force-pushed the update/paragraph-text-align branch from 0d7340d to ca3fff7 Aug 8, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Any other block left with the inline styles for these?

@gziolo

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

Any other block left with the inline styles for these?

It is the last one I think. Well, at least I don't have more PRs opened :)

We decided we don't add text-align to List block but it would be a new feature.

@gziolo gziolo merged commit b5e9613 into master Aug 8, 2019
1 of 4 checks passed
1 of 4 checks passed
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details
@gziolo gziolo deleted the update/paragraph-text-align branch Aug 8, 2019
@gziolo gziolo added this to the Gutenberg 6.3 milestone Aug 8, 2019
@koke koke referenced this pull request Aug 21, 2019
1 of 1 task complete
gziolo added a commit that referenced this pull request Aug 29, 2019
…ign (#16794)

* Block library: Refactor Paragraph block to use class name for text align

* Fix demo post to use text align class for Paragraph blocks

* Update snapshots for failing e2e tests

* Improve code readabilit for Paragraph block
gziolo added a commit that referenced this pull request Aug 29, 2019
…ign (#16794)

* Block library: Refactor Paragraph block to use class name for text align

* Fix demo post to use text align class for Paragraph blocks

* Update snapshots for failing e2e tests

* Improve code readabilit for Paragraph block
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
…ign (WordPress#16794)

* Block library: Refactor Paragraph block to use class name for text align

* Fix demo post to use text align class for Paragraph blocks

* Update snapshots for failing e2e tests

* Improve code readabilit for Paragraph block
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.