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

Try: Always collapse block alignments. #16557

Merged
merged 5 commits into from
Jul 26, 2019
Merged

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Jul 12, 2019

This PR makes the block alignments always be collapsed. This group would already collapse at mobile responsive breakpoints. In addition, this PR also adds a dropdown arrow.

This comes with a couple of benefits:

  • It ensures that the block toolbar always fits even when the item is deeply nested inside columns.
  • It affords a scalable method to show additional alignment options, such as those suggested in Blocks: Full Screen alignment/display option on several blocks #16385.
  • It scales to future ideas of allowing a theme to create CSS grid-based layouts, which could allow theme authors to create their own custom alignments such as "pull right" or others.
  • It has labels, to be descriptive of such new alignments.

Noting that 3 is just an idea at this point, but the other items on the list can potentially benefit us today.

always-collapsed-alignments

@paaljoachim
Copy link
Contributor

paaljoachim commented Jul 14, 2019

I have been thinking about this as well.
The alignment buttons take so much space in the toolbar, and to me it would be natural to have them in a drop down.

@kjellr
Copy link
Contributor

kjellr commented Jul 15, 2019

I'm all for this. It saves space, and also helps apply text labels to some often-confusing toolbar icons.

These are the sorts of toolbar controls that you're likely to hit just once or twice for any given block, so I don't think hiding them behind an extra click is too much of a hassle. Especially given the other benefits explained above.

Looks like some of the tests would need to be rewritten if we decide to land this one.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It needs some polishing but it's definitely something that will solve a couple of issues :)

@gziolo gziolo added General Interface Parts of the UI which don't fall neatly under other labels. [Feature] Blocks Overall functionality of blocks labels Jul 19, 2019
@mapk
Copy link
Contributor

mapk commented Jul 24, 2019

I'm all for this as well. With the introduction of more toolbar items, it's getting crowded. I'm just not sure what our dropdown pattern is for this stuff.

Do we use an arrow icon to indicate it's a dropdown or not? I vote yes.

Here's an example of the Table block which uses a variety.

table-dropdown

@mapk mapk mentioned this pull request Jul 24, 2019
5 tasks
@karmatosed
Copy link
Member

My concern is would be hiding a useful thing away and as a result, causing it to either be thought to not exist, happen from this suggestion? If people agree that's not the case it's totally worth exploring.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Jul 24, 2019
@gziolo
Copy link
Member

gziolo commented Jul 25, 2019

Do we use an arrow icon to indicate it's a dropdown or not? I vote yes.

Yes, 100% agree. Everything that opens a dropdown should have a consistent indicator. I also think that this should be handled on the component side rather than through CSS as proposed in this PR. (internally it can use CSS approach, it doesn't matter, my point is that this should be the default which you can opt-out from)

@gziolo gziolo force-pushed the try/always-collapsed-alignments branch from 8f12cf0 to afc3756 Compare July 25, 2019 13:50
@jasmussen jasmussen requested a review from Soean as a code owner July 25, 2019 13:50
@gziolo
Copy link
Member

gziolo commented Jul 25, 2019

Changes applied:

Screen Shot 2019-07-25 at 15 51 21
Screen Shot 2019-07-25 at 15 51 29
Screen Shot 2019-07-25 at 15 51 44
Screen Shot 2019-07-25 at 15 51 49

This resolves a few issues discussed:

  • all controls which use dropdown have arrow indicator appended
  • all controls which have items to pick with dropdown have every word starting with uppercase for consistency
  • all controls have tooltip label which starts with uppercase and the rest is lowercase for consistency

Issues to resolve

I think there are blocks like Columns which should have center block alignment added because it's the default state for all blocks. At least we use center icon this way. See:

Screen Shot 2019-07-25 at 15 01 02

@@ -69,6 +69,7 @@ function HeadingEdit( {
<HeadingToolbar minLevel={ 1 } maxLevel={ 7 } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
<p>{ __( 'Text Alignment' ) }</p>
<AlignmentToolbar
isCollapsed={ false }
Copy link
Member

Choose a reason for hiding this comment

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

It's temporary for backward compatibility. It should be resolved with #16682.

Joen Asmussen and others added 4 commits July 25, 2019 18:57
This PR makes the block alignments always be collapsed. This group would already collapse at mobile responsive breakpoints. In addition, this PR also adds a dropdown arrow.

This comes with a couple of benefit:

- It ensures that the block toolbar always fits even when the item is deeply nested inside columns.
- It affords a scalable method to show additional alignment options, such as those suggested in #16385.
- It scales to future ideas of allowing a theme to create CSS grid-based layouts, which could allow theme authors to create their own custom alignments such as "pull right" or others.
- It has labels, to be descriptive of such new alignments.

Noting that 3 is just an idea at this point, but the other items on the list can potentially benefit us today.
@gziolo gziolo force-pushed the try/always-collapsed-alignments branch from afc3756 to a93b328 Compare July 25, 2019 17:04
@gziolo gziolo force-pushed the try/always-collapsed-alignments branch from a93b328 to 66a5518 Compare July 25, 2019 18:06
@gziolo gziolo self-assigned this Jul 25, 2019
@gziolo
Copy link
Member

gziolo commented Jul 25, 2019

@mapk and @karmatosed, I managed to update all e2e tests to work with the updated UI for all types of alignment controls which can be added to the block toolbar. I'd like to move forward with this PR as this gives are more space to work on further refinements for text blocks as described in #15096.

@mapk
Copy link
Contributor

mapk commented Jul 25, 2019

I just tested these all out, @gziolo, and they look and work great! Let's get this merged.

collapsed

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I consider it ready to go feature-wise. Given that I added most of the code changes, I would appreciate sanity check from another developer as well because I had to rework a few e2e tests to make Travis happy :)

I also opened #16764 so we could discuss how labels/titles should be capitalized.
There is also one possible follow-up task where we should add center alignment to those blocks which offer only full and wide alignments if we are concerned by the fact that no alignment selected is in fact center. However, I'm also fine with leaving it as is to avoid setting explicitly center when someone selects it.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Let's try that.

@gziolo gziolo merged commit 79d7847 into master Jul 26, 2019
@gziolo gziolo deleted the try/always-collapsed-alignments branch July 26, 2019 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants