-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
FormTokenField: Add Help Text #15469
Conversation
margin-bottom: $grid-size-small; | ||
} | ||
|
||
.components-form-token-field__help { | ||
display: block; | ||
font-style: italic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have some design feedback about the italic style. Personally, I'd avoid it because some users find it difficult to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made it italic based on styling of the help text of BaseControl
which is used as a base for most of other controls. If there are italics missing for help texts similar to this, they are inconsistent with the rest. Just a few examples:
I agree on the issue with italics but I think we should address that separately and on a global level for all those existing usages of BaseControl. What do you think about leaving this PR as italics and opening a discussion to replace them globally for all help texts? I'll be happy to make that PR once we settle on a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think italics vs. no italics can be discussed separately — this PR is just mirroring the existing styles used in .components-base-control__help
.
(From some quick clicking around, it seems that the Permalink help text is the only text of this kind that isn't italicized, so we should make a decision one way or another).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Italic is terrible for some kind of dyslexia when used on whole sentences. I vote for just not add new Italic. Also, planning to create related GitHub issue and Trac ticket. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool — I'm fine getting rid of the italic globally if we can replace it with something else. I just want to make sure that help text appears differently from the form field label so users can tell them apart. But there are other ways to do that besides making the text italic. I'll keep an eye out for the ticket and try out some ideas there. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are other ways to do that besides making the text italic
Definitely agree. One day before I die I'd like to see a typographic system in WordPress 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened #15683 to track the inconsistent help text, and to consider the use of italic text there. We'll get that sorted out globally in Gutenberg. 🙂
In the meantime, that shouldn't block this PR since it's not adding any new styles — just replicating the existing BaseControl
help text style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @marekhrabe, I left some comments.
Re: the Italic text, I'd like some design feedback. I'd like to avoid it as much as possible for better readability. Also, seems to me Gutenberg isn't using Italic for this kind of hints underneath input fields, for example:
/Cc @karmatosed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this component has a label and a help text feature offered by the BaseControl component: https://github.com/WordPress/gutenberg//blob/1e40b281d1db725e512e383b5e00f3b59e333d5a/packages/components/src/base-control/README.md
Would it be possible to use the base control here, avoid the need for label and help text custom styles and promoting code reusability?
<div id={ `components-form-token-suggestions-howto-${ instanceId }` } className="screen-reader-text"> | ||
{ __( 'Separate with commas' ) } | ||
</div> | ||
<p id={ `components-form-token-suggestions-howto-${ instanceId }` } className="components-form-token-field__help"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the id components-form-token-suggestions-howto-${ instanceId }
is not being referenced anywhere, can we remove the id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've actually tried using BaseControl but it cannot be easily used as this component attaches a lot of additional attributes on the wrapping element and that's not supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marekhrabe This is looking great. Just a tiny design change before it's ready to merge on my end. Currently, the first version of the label says:
Separate using commas or the Enter key
We should add a period at the end there to align with other help text. Unfortunately, that awkwardly pushes the word "key." onto its own line on desktop screens. 😄
To avoid that, let's swap out "using" with the word "with":
We can also make that change with the second label version that includes the mention of spaces:
919e436
to
3dd54a2
Compare
What a fix! 😀Message updated, code rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great from a design perspective. 👍
👍 from me. |
Thanks, @afercia. Can you please update the review too? Github still blocks me from merging as you requested changes previously.
|
Description
Fixes #15355
I've made the message visible and added the mention of the enter key. It's styled to look the same way as other help texts in the sidebar.
The component allows developers to opt-in for using spacebar as a separator too. I've reflected that and made two versions of the help text based on the configuration.
How has this been tested?
The sidebar term selector (Tags, Categories) is the only place where this component is used in core.
Screenshots