Add a "separator" attribute to post-terms block #32812
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: +141 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Changed the default from a pipe ( |
|
This works great in my testing.
The only thing I'm not sure about is whether this needs to live in the Advanced section — since this block doesn't have very many controls to begin with, I think it might as well be highlighted elsewhere in the sidebar. Adding |
|
This reminds me a lot of #23293 The designs in that issue all seemed to default to showing this option in the sidebar and also included a spacing and size option — all under it's own accordion section:
-- I think it could make sense to expose something like this in a block's toolbar, and the post-terms block is a good example. There are minimal controls in the toolbar already, and controlling the separators feels important enough to the block's needs to show in the toolbar. With that in mind, I wonder if there's a way to have one UI that will work both in the sidebar and in the toolbar. |
Something like this will be needed. It's not clear to me what the suggested design is though? @shaunandrews is this the design suggested to be explored but in the toolbar? If yes, even though it might be something |
aristath
force-pushed
the
add/post-terms-separator
branch
from
f606ec6
to
727ae05
Compare
|
"Spacing" makes sense, but I would skip "size" to keep things more straightforward. I don't think this belongs in the toolbar. |
aristath
force-pushed
the
add/post-terms-separator
branch
from
463c2c2
to
51d2535
Compare
aristath
force-pushed
the
add/post-terms-separator
branch
from
51d2535
to
1483bf8
Compare
mtias
reviewed
Jul 8, 2021
| }, | ||
| "separator": { | ||
| "type": "string", | ||
| "default": ", " |
There was a problem hiding this comment.
I think a comma makes more sense — it tends to be the most commonly used separator in themes.
ntsekouras
reviewed
Jul 9, 2021
ntsekouras
reviewed
Jul 9, 2021
There's nothing stopping us from adding spacing controls in future iterations, but I'm unsure whether that would be a requirement or not for a 1st iteration. The "separator characters(s)" control would always be needed regardless of whether we have spacing or not, so IMO it would be safer to go with something small and if we see that spacing is required and requested, then add it. Personal opinion: I was thinking that the spacing control would probably add too much complexity for something that may not necessarily need that much... The space character may not be directly visible when editing the block, but when entering the control and actually editing the value, it becomes self-evident and is extremely simple to use. Granted, it's not perfect, but maybe it's the lesser of 2 evils? As a user I'd rather hit the spacebar once or twice than have to edit a bunch of separate controls 😅 |
|
Yeah — assuming we stick with the comma as a default separator, I think a spacing control is unnecessary for this iteration. |
|
Okay then, let's land this first iteration as is, but first we should add a style to the separator to preserve Since JSX removes whitespace. You can try this by just adding more than one space in separator. |
ntsekouras
reviewed
Jul 9, 2021
Great catch! Done in e5c8fb8 👍 |
ntsekouras
approved these changes
Jul 9, 2021
There was a problem hiding this comment.
Thanks Ari! I'm pre approving but you still need to just import the new style here: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/style.scss
That's all that is left.
--edit
I pushed the import 😄
aristath
force-pushed
the
add/post-terms-separator
branch
from
68cc112
to
d5bb6b2
Compare
sarayourfriend
pushed a commit
that referenced
this pull request
Jul 15, 2021
* Add a "separator" attribute to post-terms * Change default from a pipe to a comma * fix defaults * Add separator class * update fixtures * change selector from sep to separator * Add styles * indentation fix * import style Co-authored-by: ntsekouras <ntsekouras@outlook.com>
12 tasks
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
Fixes #31710
Adds a "Separator" attribute to post-terms. The control is exposed in the "Advanced" section of the post-categories and post-tags blocks.
How has this been tested?
,and confirmed that it works both in the editor and the frontendChecklist:
*.native.jsfiles for terms that need renaming or removal).