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

Nav Block: adds basic items justification controls #18909

Merged
merged 14 commits into from
Dec 5, 2019

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Dec 4, 2019

Closes #18557.
Closes #18898.

Enables the ability to justify the items within the Nav Block to the:

  • left (technically flex-start)
  • center
  • right (technically flex-end)

This is not the same as alignment controls which actually shift the entire Block. This approach was avoided because:

  1. When left or right alignment was applied the whole Block became "full width".
  2. It was not possible to have a block which was both full-wide and with the items aligned to the left/right.

The approach in this PR works around that by adding a custom justification control which controls the alignment of individual items (taken as a whole) rather than the Block as a unit.

Questions

  • Do we need custom icons for item justification or are the text justification ones we're using ok?
  • Shall we add further flexbox layout options for space-around, space-between...etc? I'm tempted to say we save that for a future PR and just get the basics merged.

How has this been tested?

Before testing/reviewing please read the background discussion in the Issue to understand the motivation behind the justification approach.

This has been manually tested.

  1. Add Nav Block.
  2. Add lots of items.
  3. Use controls to justify the items.
  4. Use alignment to modify the alignment widths whilst also toggling the item justification.
  5. Check visual output corresponds correctly in both the Editor and on the front of site (Theme).

Deprecation Testing

This PR doesn't need to add a deprecation because we don't change the output of the Block until the user selects a justification option (ie: the default is don't change anything).

You can test this by

  1. Switch to master.
  2. Add Post with Nav Block with items.
  3. Switch to this branch.
  4. Reload your Page from #2.
  5. Ensure the Block does not error.

Screenshots

Screen Capture on 2019-12-04 at 12-41-01

Types of changes

New feature (non-breaking change which adds functionality)

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@getdave getdave added [Package] Block library /packages/block-library [Block] Navigation Affects the Navigation Block labels Dec 4, 2019
@getdave getdave self-assigned this Dec 4, 2019
@getdave getdave marked this pull request as ready for review December 4, 2019 12:41
@jasmussen
Copy link
Contributor

Thank you for the clarification. Alignment controls don't seem to add a ton of value given how the horizontal width of the block can grow. But justification controls do make sense. You might create a 2 column layout with a logo in the left column, a nav block in the right column, and then wanting to justify the items inside the nav block to stack towards the right.

In that vein, 👍 👍 on the feature.

I wonder where in the toolbar the alignments should sit, they are almost never at the end. It should at least sit before colors, should it also sit before block navigator? I'd tend to think so. The overall pattern hasn't been fully established, but usually it goes like this

[switcher] [alignments] [inline] [plugins] [ellipsis]

So starting with block level first and moving towards inline and "other".

@getdave
Copy link
Contributor Author

getdave commented Dec 4, 2019

@jasmussen Thank you for the 👍 on the feature.

I've updated the toolbar order. Good spot!

@retrofox retrofox self-requested a review December 4, 2019 15:55
Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

Choose a reason for hiding this comment

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

Haven't actually tested the code yet, but I noticed some minor things that could be improved.

packages/block-library/src/navigation/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/navigation/style.scss Outdated Show resolved Hide resolved
packages/block-library/src/navigation/edit.js Show resolved Hide resolved
@retrofox
Copy link
Contributor

retrofox commented Dec 4, 2019

I don't see the wide-width, full-width toolbar in the current PR. Are we going to add (another) alignment toolbar? ( I see both in your gif animation)
These are mostly layout options rather than alignment ones. Anyway, I wouldn't like to bring and open this topic (again 😅 )

if we are not going to add two toolbars and just adds only one, could extend the current navigation adding these justify options? I've copied/pasted what you wrote straight in the navigation block.

packages/block-editor/src/components/block-alignment-toolbar/index.js

const BLOCK_ALIGNMENTS_CONTROLS = {
	// ...,
	justifyLeft: {
		icon: 'editor-alignleft',
		title: __( 'Justify items left' ),
	},
	justifyCenter: {
		icon: 'editor-aligncenter',
		title: __( 'Justify items center' ),
	},
	justifyRight: {
		icon: 'editor-alignright',
		title: __( 'Justify items right' ),
	},
};

packages/block-library/src/navigation/index.js

export const settings = {
	// ...,

	supports: {
		align: [ 'justifyLeft', 'justifyCenter', 'justifyRight' ],
		// ...,
	},

	// ...,
};

After that, it's a matter of defining the CSS on both sides.

Sorry if this suggestion changes considerably this approach, but I think it worth because it's simpler since re-using blocks.

@getdave
Copy link
Contributor Author

getdave commented Dec 5, 2019

@retrofox Thanks for your comments. Allow me to address them.

I don't see the wide-width, full-width toolbar in the current PR.

The alignment toolbar is not in the edit.js because it gets added automatically via the supports.align config in index.js. You don't have to code it manually.

Are we going to add (another) alignment toolbar?

In this block yes there are two things that control placement on the hoz axis:

  1. Alignment - this weirdly controls the width even though it's called "alignment".
  2. Justification - this is distinguished from "alignment" because it's justifying the items on the hoz axis not the Block itself. 🤷‍♂

I think this might be a use case specific to Nav. I'm not sure.

...if we are not going to add two toolbars and just adds only one, could extend the current navigation adding these justify options?

No because of the reasons outlined in the PR description, namely:

  • If you have a single toolbar you can't select more than 1 option at a time. You might want full width + justified right for example.
  • Alignment and Justification are not the same thing and colocating them under 1 toolbar could be confusing especially if alignLeft/Right/Center was added.

My feeling at this stage is that I'm keen not to over abstract justification controls into a reusable toolbar too soon. I don't yet know of any other use cases where this functionality would be required. Moreover, this feature needs to be land in the Nav Block asap.

Going off down a reusable route will introduce additional complexity. I'd be happy to handle that in a follow-up PR but my instinct is that it's premature to address it here.

As always, I'm open to being told I'm wrong! 😄

@getdave
Copy link
Contributor Author

getdave commented Dec 5, 2019

@jasmussen @mtias We now have the new icons in place.

@getdave getdave closed this Dec 5, 2019
@getdave getdave reopened this Dec 5, 2019
@getdave getdave requested a review from mtias December 5, 2019 09:53
@mtias
Copy link
Member

mtias commented Dec 5, 2019

It doesn't seem to be affecting the display:

image

Previously the nav inner elements didn’t occupy the full space of their container. As a result whilst the elements were justified it didn’t appear so because the containing element was only as wide as the width of the items.
@getdave
Copy link
Contributor Author

getdave commented Dec 5, 2019

It doesn't seem to be affecting the display:

image

@mtias This caught me out. Apologies. I was stress testing with a large nav with lots of items. This caused me to miss the issue.

The issue was the nav inner containing elements didn’t occupy the full space of their container. As a result whilst the elements were justified it didn’t appear so because the containing element was only as wide as the width of the items.

This is fixed.

Screen Shot 2019-12-05 at 11 42 10

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

I tested and it worked very well!

@getdave
Copy link
Contributor Author

getdave commented Dec 5, 2019

@draganescu Thanks for your review 👍

@ZebulanStanphill Please could you review your review as it's now blocking merge? Much appreciated.

Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

Choose a reason for hiding this comment

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

Everything seems to be working as expected. 👍

Copy link
Member

@obenland obenland left a comment

Choose a reason for hiding this comment

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

Nice work @getdave!

@shaunandrews
Copy link
Contributor

I saw the new "Justify" settings for the first time today and was completely confused. It feels like we're making a distinction between "justifying" and "aligning" for reasons that most people will never understand. I've never seen the word "justify" used like this before, and would not know what it meant. "Align" would be way more obvious to me.

The use of new icons is also introducing more overhead. These icons don't help me understand that I can align my navigation items to the left or right. To me, they look more indent.

@ZebulanStanphill
Copy link
Member

@shaunandrews I think part of the issue is that there is already a block alignment toolbar, but it doesn't really contain any actual alignments except for aligncenter. alignleft and alignright are floats, and the wide and full options similarly have nothing to do with "alignment". The options really feel more like "flow" options that affect how the block is positioned in the document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Package] Block library /packages/block-library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation Block Doesn't Output Alignment Classes Navigation Block: needs alignment controls
9 participants