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

[Fix] Make sure Latest Posts alignment class behaviour #8847

Merged
merged 2 commits into from
Aug 13, 2018
Merged

[Fix] Make sure Latest Posts alignment class behaviour #8847

merged 2 commits into from
Aug 13, 2018

Conversation

nfmohit
Copy link
Member

@nfmohit nfmohit commented Aug 10, 2018

Description

This PR closes #7911 which reports the incorrect behavior of the alignment classes in the Latest Posts block, where it applies alignment classes even if no alignment was selected. It also gets rids of the behavior where the center alignment is selected by default.

How has this been tested?

This PR has been tested by going through the following steps:

  1. Started a new post using the Gutenberg editor.
  2. Added the Latest Post block.
  3. Selected the center alignment option in the block toolbar.
  4. Deselected the center alignment option in the block toolbar.
  5. Published the post and made sure no alignment classes are added to the block, as the alignment was deselected.

This was tested in WP 4.9.8, Gutenberg 3.5.0, Apache server with PHP 7.2.0 and MySQL 5.6.34. According to initial tests, the code doesn’t seem to affect any other areas.

Screenshots

pull-7911-min

Types of changes

This PR introduces a conditional before applying the alignment classes to make sure an alignment is selected in the first place. It also removes the default property from the align attribute, so that the center alignment isn't selected and applied by default.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@aduth
Copy link
Member

aduth commented Aug 13, 2018

Thinking about the original issue, if it was really intentional for center to be default, and how we'd support "unsetting" that value, I think if that was something we'd want to support, we could do so by setting the alignment to null instead of undefined. As undefined, I think it's sensible that the default value take effect.

This, of course, is a non-issue after these changes.

@aduth
Copy link
Member

aduth commented Aug 13, 2018

Totally unrelated to the changes here, but the alignment implementation doesn't work so well on lots of themes, where margin: 0 auto; on an element which is full-width already doesn't do anything (i.e. doesn't appear to be centered). But the class is there.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@aduth
Copy link
Member

aduth commented Aug 13, 2018

Failing test was legitimate. => assignment alignment needed to be updated. Pushed change in c1fabd2.

@aduth aduth merged commit 070c26f into WordPress:master Aug 13, 2018
@nfmohit
Copy link
Member Author

nfmohit commented Aug 15, 2018

Thank you so much for the correction and merge @aduth ❤️

@nfmohit
Copy link
Member Author

nfmohit commented Aug 15, 2018

@aduth Any idea why we don't use flex to center the list items instead of margin: 0 auto; which mostly doesn't work.

1

That would, of course, require us to wrap the list up with a div wrapper. Let me know if you want me to submit PRs with that. Thanks ❤️

@aduth
Copy link
Member

aduth commented Aug 15, 2018

@nfmohit-wpmudev

A few things:

  • Apparently we don't add anything in the editor to reflect center alignment? That might be a good first step 😄
  • There's already a fair bit of inconsistency in alignment options across blocks, observed at Unify the block toolbar for all text blocks #3785 . The first point and this issue could be jointly remedied by using the align supports
  • The margin: 0 auto; is the implementation of .aligncenter, which predates Gutenberg. I imagine the reasoning for this may be broader browser support: While wp-admin supports only IE11 and newer, we need to be more careful about the content we produce. I realize there's some hypocrisy here in that the Columns block uses Flex. Mentioning less as a reason to "not do it", but more as context for why it may not have been used.

@nfmohit
Copy link
Member Author

nfmohit commented Aug 16, 2018

Got it, thank you so much for the explanation @aduth ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alignment: latest posts block reapplies aligncenter on save even if it was deselected
2 participants