Abstract Justification Toolbar to it's own block-editor component #28439
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mkaz
added
[Block] Navigation
|
Size Change: +243 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Cool work, looks and works as advertised:
Code looks good and this is a nice enhancement. I CC'ed Nik for a quick sanity check as I know he's been looking at adjacent things. One question, though, about this code: I know this is existing code that has shipped for months and months, so it's probably not feasible to course correct. But what this code tells me is that you can't use justification controls unless the child elements are inside an unordered list. Is there any way to change this to be less opinionated about what elements are being used? Could be fine as a followup if possible. |
ntsekouras
reviewed
Jan 25, 2021
There was a problem hiding this comment.
Thanks for working on this @mkaz !
Maybe by including an integration in another component as well, besides Navigation, will be better, as it will help identify more things or problems we have to solve.
|
@ntsekouras @jasmussen So looking at the CSS we could probably tackle it a couple of different ways, but tricky for implementation due to existing blocks, classnames, and markup. So, what I'm thinking is don't include any CSS in the controls at let the component using them implement as needed. This is how it works on navigation, the justify controls takes an Looking at how I would implement the same controls with CSS for buttons is tricky sine the buttons component uses a different set of classnames, so changing either way will have some deprecation issues. The downside is the controls aren't simple to drop-in and you automatically get all the classnames needed. Thoughts? |
This seems right to me. There will likely be enough use cases for justification controls, that the markup can't always be the same anyway. But I will defer to Nik! |
Maybe it's better this way yes. We can revisit if needed, when applying this control to other blocks. |
|
We could write a "best practices" and suggest some good defaults, so the inconsistent markup behavior doesn't grow out of hands, in case we want to revisit? |
|
Thanks, I've updated the component and I think it is ready. Joen, good suggestion on the documentation, I updated the readme to I hope clarify. I'm keeping the CSS with the component, this will help standardize people using the same classnames when used. It will be left to the consumer of the controls to place the classname where needed, and we can revisit after seeing how it used and adjust. |
ntsekouras
approved these changes
Feb 1, 2021
ntsekouras
reviewed
Feb 2, 2021
- Updates style moving to component - Fix exporting for component - Initial stubbed documentation
- Simplifies CSS for toolbar to be generic, not just ul tag. - Adds CSS back to navigation block since it is needed for > ul - Updated documentation for using controls
mkaz
force-pushed
the
update/justify-comp
branch
from
f8439b1 to
247a6f8
Compare
youknowriad
reviewed
Feb 15, 2021
| 'space-between': justifySpaceBetween, | ||
| }; | ||
|
|
||
| export function JustifyToolbar( { |
There was a problem hiding this comment.
Is the name good enough for a public API? Is it consistent with other *Toolbar components? Maybe it is, just making sure it's considered carefully.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Move the toolbar created for the Navigation block to add justification controls, mainly space-between, to it's own component within the block-editor components. We want to add these controls additional to the Social Links block, but first need this PR to abstract it out and then can use in both spots without duplicating work.
How has this been tested?
Types of changes
No functional changes, just code organization and clean up making the component reusable.