Try: Allow space between on menu items. #28169
Merged
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
|
Size Change: +91 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
|
This will solve #27132 |
At the very least, I think the justification setting should not display for the vertical variation. It would be nice to add a height option to the vertical variant, and then allow for vertical spacing options. |
jasmussen
force-pushed
the
try/nav-space-between
branch
from
93d95c3
to
8823ef6
Compare
|
I updated the "menu" icon to no longer use this Dashicon:
But to use one from the WordPress library:
Note, that menu icon should not be used in the menu at all, it should be showing the "Space Between" icon. I'm still chewing on that one, as well as the other good overnight feedback. |
|
I refactored the code to use icons from the icon library. That means the following icons are now available to use elsewhere:
One hiccup, and it explains why the space between icon does not show up as active when selected. The following code sets the active icon in the toolbar: What it does is basically compare the list of icons ( It works for But it breaks for I feel like this is needlessly complex, and there should be a far simpler way of matching the value to icons. @draganescu I know you worked on this, if you have a moment, any thoughts on how to best fix this? Would this work? I'll keep tinkering in the mean time, but would appreciate pointers! |
|
@mkaz Once we land this one, it would be nice to refactor this alignment dropdown so it's easy to port to Social Links, for example, that would solve #17275. CC: @ntsekouras as I think I recall you thinking about the same. |
|
Okay, I got it working:
But it's an ugly hack that can almost certainly be made much prettier. Shield your eyes: e34ef36#diff-610616a7f92cf5567753ff6e54b42e95ffd4b9607984b72c814f56279e87fff4R62 If anyone who's less tired than I am would like to help me refactor that to something nicer, I'd much appreciate it. In the mean time, this should be ready for reviews! |
draganescu
approved these changes
Jan 14, 2021
ntsekouras
reviewed
Jan 14, 2021
There was a problem hiding this comment.
Thanks for working on this Joen! I chimed in and made buttons use these nice new icons you created, for consistency.
In general as we have already discussed, we should make in the future a generic component for this: #28169 (comment)
The only reason I'm not approving now is that I pushed code as well. I believe it's ready to 🚢 though.
|
Thank you Nik, I tested this and it works as intended:
I restarted the tests, let's merge when they pass and work on the followups 👌 |
mkaz
approved these changes
Jan 14, 2021
There was a problem hiding this comment.
I left a comment about your "ugly hack" feel free to switch using lodash kebab case if you want, but I don't see it as necessary since it also adds a small dependency, either way is fine.
This looks good, and I'll review the other issues to see what is next to get this implemented for the social links. 👍
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.
This PR tries to add
space-betweento the navigation menu as an option. I also updated the icons. It works fine. Before:After:
Here are the icons:
What's missing?
Penny for your thoughts?