CSS Class Names: Add dropdown to choose from block styles #34521
Conversation
|
Size Change: +574 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
try/block-style-name-drop-down-controls
branch
from
fd2df0c
to
3ce7ae7
Compare
|
We might need the
Also, alignment of the icon is a little tricky here. We'd want to align the ellipsis icon with the top (the Additional CSS class(es) label) or with the input field. But because we're using It does look much neater in the designs at #33934 – where the ellipsis icon is aligned with the input field – but in order to achieve this we might have to either:
No 2 runs the risk of losing standardization since all items in the Advanced panel use this component. No 1 would have be thoroughly tested given the frequency with which |
ramonjd
force-pushed
the
try/block-style-name-drop-down-controls
branch
2 times, most recently
from
7be19d0
to
37aae17
Compare
ramonjd
added
[Feature] Custom Editor Styles
|
I pushed a few updates. First I updated the menu label from "Block style classes" to "Existing styles." Then, I decided to be reckless and switch from
Otherwise, this PR seems to work as expected; The input respects the styles from the accordion above; I'm able to choose from the menu to switch styles; Typing in a class manually correctly selects the existing class. |
|
Thanks for the idea @shaunandrews I think we can work with it. I've taken it a bit further in fa22229 by passing down the dropdown as a child to Input overflow: Small screens: Label overflow: Passing down the dropdown as a child to If you think this approach is okay, I might need to split this PR and do that bit first since I'd have to update the stories and promote the benefits 😄 Result:
|
ramonjd
force-pushed
the
try/block-style-name-drop-down-controls
branch
2 times, most recently
from
fd15e64
to
74eceee
Compare
|
There's a slight mis-alignment (1px too high, I think), but otherwise this seems good.
Seems like a solid step forward. |
|
Thanks for testing @shaunandrews 🙇
Pah! 1px, 2px... Out of interest, which browser are you using? Here's Chrome, Safari and Firefox for me, in order:
|
|
Safari. I may or may not have zoomed in so my old eyes can see things. |
Good call. When I zoom in or out by various factors, it's visible.
|
Eyeballing extra margins Ensuring the additional class input field stretches in mobile
…This might be a terrible idea.
…l component so we can position the icon more accurately. We're also adding a background color to the icon in case the underlying text flow beneath it. Finally, in the case of smaller screens, we're bumping up the size of the icon.
| @@ -65,6 +66,7 @@ function TextControl( | |||
| ref={ ref } | |||
| { ...props } | |||
| /> | |||
| { children } | |||
There was a problem hiding this comment.
By way of explanation, rendering children here was a trade off.
Following the design requirements, we need to be able to add a dropdown menu as a sibling of the text control input.
We could have created a custom component using the constituent parts of <TextControl />. The Advanced panel, however, uses <TextControl /> multiple times, which means that updates would have to be copied over to the custom component to retain consistency.
Does adding children here contravene any component guidelines from a technical or philosophical standpoint?
As far as I can tell, there are no side effects to adding and using the children prop, though I am worried that it might be "impure" in that it invites manifestations of a base component.
I'm overthinking things, yes.
There was a problem hiding this comment.
I'd be grateful for your thoughts here @ciampo whenever you get time. 🙏
There was a problem hiding this comment.
Thank you for the ping! Always happy to advise / give direction on components :)
I would say that rendering children inside a TextControl goes a bit against the way the component is supposed to work. TextControl, in my mind, is supposed to be an enriched input field.
I would personally recommend that, instead of adding children to TextInput, you render the dropdown menu as a sibling of TextInput in the CustomClassNameControl component.
Also, cc'ing @diegohaz and @mirka in case they wanted to add their views here as well
There was a problem hiding this comment.
Thanks for the feedback and for confirming what the tiny voice in my brain was trying to tell me. I was truly struggling with the conflict 😆
I would personally recommend that, instead of adding children to TextInput, you render the dropdown menu as a sibling of TextInput in the CustomClassNameControl component.
I'll try that out, cheers! If the other Advanced control inputs end up looking or working differently I can try to switch them all over to TextInput as well.
There was a problem hiding this comment.
Ah, I just realized that TextInput isn't a component yet. Or at least we haven't migrated it from g2.
I should know, I started it and never finished! 😅
There was a problem hiding this comment.
Ah, I just realized that TextInput isn't a component yet. Or at least we haven't migrated it from g2.
We did initially look into it as well after you started it, but we later stopped because it was a very complicated task, since TextInput is very complex (it tries to handle single/multi line text and number input) and represents a very different approach to what we currently have in Gutenberg.
There was a problem hiding this comment.
We did initially look into it as well after you started it, but we later stopped because it was a very complicated task
Thanks for the background info. 🙇
I discovered personally how tricky it was 😬 so I feel better that folks with a lot more experience than I have also felt the same!
- Added prop notes for children in readme - Added story for withChildren
ramonjd
force-pushed
the
try/block-style-name-drop-down-controls
branch
from
74eceee
to
de6dd44
Compare
|
@shaunandrews How would you feel if I parked this one for now? I'm not sure how to achieve the current design without re-creating TextControl, which seems like an overkill. |
|
Closing for now until we can revisit the approach. We can always reopen. Thanks to everyone for their help on this one! |
Description
Resolves #33934
These changes allow us to switch between block styles beside the advanced controls, and offer a quick way to select or reinstate block styles classes.
Testing
Please make yourself comfortable.
Insert a block that has block styles, for example the Image, Table or Buttons Block in TT1 Blocks theme.
Check that:
Check TextControl in Storybook for a new story (
withChildren);Types of changes
Additional functionality.
Checklist:
*.native.jsfiles for terms that need renaming or removal).