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

Archives block: use align supports flag + add wide/full align support #10706

Closed
wants to merge 1 commit into from
Closed

Archives block: use align supports flag + add wide/full align support #10706

wants to merge 1 commit into from

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Oct 17, 2018

Description

Closes #11416.

This PR updates the Archives block to use supports: { align } to add most of its alignment support, and it adds support for the wide and full alignments to the Archives block.

Notably, the align attribute is still defined in PHP since the enum property prevents invalid alignment classes from appearing on the front-end. (You can test this by using the Code Editor to modify an existing align attribute on the block to have an invalid value and then checking the markup of the resulting post.

Types of changes

  • Using supports: { align } to add block alignment support to the block.
  • Added enum to the align attribute defined in index.php to fix an issue in master where you could set invalid values for the attribute, which would result in invalid classes (e.g. "alignbob") on the front-end.
  • Some imports in edit.js were incorrectly listed as "Internal dependencies" (which they may have been prior to some stuff being moved into packages). This has been fixed, and the imports are now in alphabetical order.

Related PRs

@ZebulanStanphill ZebulanStanphill changed the title Archives block: use align supports flag + wide/full align support Archives block: use align supports flag + add wide/full align support Oct 17, 2018
@earnjam earnjam added [Type] Enhancement A suggestion for improvement. [Block] Archives Affects the Archives Block [Type] Code Quality Issues or PRs that relate to code quality labels Nov 2, 2018
@ZebulanStanphill
Copy link
Member Author

Rebased PR and updated it based on feedback on #10758.

@gziolo
Copy link
Member

gziolo commented Jan 28, 2019

@melchoyce or @jasmussen - is it expected to add align wide and full for the Archives block? In #9469 @nfmohit-wpmudev proposed the opposite for Category block.

@gziolo
Copy link
Member

gziolo commented Jan 28, 2019

Now, I see #9413 opened by @kjellr where it's suggested the opposite:

Since these blocks are similar in function and appearance, we should sync up these alignment options. To do so, I suggest we remove the "Full" alignment from the Categories block — it doesn't seem like it'd be a frequently used setting for a list.

It looks like we should proceed with #9469 instead.

@ZebulanStanphill
Copy link
Member Author

@gziolo But my previous PR, #13215, was already merged by @aduth, adding wide align support to the Categories block. That PR runs counter to #9413, since my PR actually added an alignment to the Categories block. Perhaps some kind of official decision needs to be made on all this? It doesn't make sense to go halfway in one direction and then halfway in the other.

@kjellr
Copy link
Contributor

kjellr commented Jan 28, 2019

@ZebulanStanphill

But my previous PR, #13215, was already merged by @aduth, adding wide align support to the Categories block.

Yes, my ticket there is now out of date. I just updated the description there to reflect that.

@gziolo

Now, I see #9413 opened by @kjellr where it's suggested the opposite:

My gut still says that text or list-based blocks like these shouldn't ship with wide/full alignments. They don't clearly benefit from the additional space: in fact, they become less readable with longer line length.

@melchoyce
Copy link
Contributor

My gut still says that text or list-based blocks like these shouldn't ship with wide/full alignments. They don't clearly benefit from the additional space: in fact, they become less readable with longer line length.

+1

@gziolo
Copy link
Member

gziolo commented Jan 29, 2019

In that case, I'm closing this PR :(

Let's make sure we remove full and wide from other blocks where it doesn't make sense.

@ZebulanStanphill sorry about that. Thank you for your work, hopefully, it pays off with more consistent UI as an effect of your contribution. 🙇

@gziolo gziolo closed this Jan 29, 2019
@ZebulanStanphill ZebulanStanphill deleted the update/archives-block-align branch January 29, 2019 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Archives Affects the Archives Block [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Archives block: add wide and full alignment support
5 participants