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.
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
Interface guidelines for forms #261
Interface guidelines for forms #261
Changes from 4 commits
5cc8cac
4c7222d
e8b570a
2a62bd5
0f1bc44
e06b2c6
3804436
a202e05
3c6e364
a373809
32f4e6e
786bdcd
3a858fe
cee08d2
67c15b3
d6e29a4
62492d4
91498ae
5a26f08
8664bde
d5e2d4d
d201897
3cd148e
1333064
a607378
0c55099
5758eba
b4ff765
9b6b785
e1b6e2c
8a4ad2c
0d0c6c5
265a168
90544e0
4e2265b
32e8aad
fe1aa91
585268e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@mperrotti Can we simplify this leading description and make it into a single paragraph?
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.
Is a search field part of a forms pattern? Search fields appear throughout dotcom, and they're not rare =).
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 could be, but usually it isn't. I was trying to think of the most common and obvious example.
Do you (or any other reviewers) have any better examples?
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.
@mperrotti I'm not sure. It's confusing to see an example about a search field while this page is specifically about [long-form] forms pattern.
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.
Good point about these guidelines being for long-form forms. I think it might be best to exclude this paragraph. Visually hidden labels can be addressed in form component guidelines.
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.
The examples using checkbox groups have some weird application of bold. Do we really want to use bold labels for every checkbox or radio button item? Are the validation messages also meant to be bold? 🤔
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.
These are all meant to be bold. That's how the components are currently designed.
I'd be happy to change the checkbox and radio labels from bold to regular weight in separate PCSS and PRC PRs. What do you think @vdepizzol?
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 think the intent with the semibold
label
is that the styles are consistent through all form elements (soinput
,select
etc.) But I don't feel that the validation messages should also be semibold.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.
The semibold error messages are kind of new: primer/react#2013
@colebemis and I think it helps the text feel more balanced with the leading icon, but I'm open to changing it back to regular weight it if it's causing an issue.
Other reviewers - please chime in if you agree with @langermank that validation text should go back to regular weight.
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.
@mperrotti I'd keep it at a regular weight. Validation messages can appear multiple times to the user in the same form. The bolded font feels too heavy.
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.
Ok, I'll update the example images and submit a PR to PRC to change the weight back to normal.
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.
The radio button and checkbox groups seem to have inconsistent headings and unnecessary bold text. Could we revisit these design decisions?
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.
Yes, let's revisit. I think the checkbox and radio labels should be the same weight as body text.
@vdepizzol - could you open an issue or discussion?
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'm not sure I understand what this image is telling here —
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 think you're referring to the labels in the first three form controls as "headings". None of these are headings, they are input labels.
Horizontal placement is just one of the ways you can visually group two or more inputs.
I'm not sure I understand this question.
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 updated the image, but it's still a little "squished" because of the annotations.
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.
This is the first time I'm being introduced to the
internal label
concept, I wonder if this is something we should highlight above, on theLabels
section?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.
This is only something we'd do with an
Action menu
component, so that information will go in that component's guidelines.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.
This UI feels confusing, with the checkbox label and text input label having the same alignment and style. I don't think we should use a search icon in that case, but I'm afraid that's not the only thing that's adding to the confusion.
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.
You're right - this isn't a great example. I'll come up with a better way to show a labeled input that is "nested" and progressively disclosed.
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.
@mperrotti I'm finding it quite confusing to see these 3 gifs next to each other. There's a lot going on 😬 . I'm not sure I have any recommendations for now, but it's something we should pay more attention...
Screen.Recording.2022-05-23.at.10.49.45.mov
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.
You're right - all this motion is overwhelming. I'm going to try converting these to
<video>
elements that can be played or paused individually.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.
Is this the right metric to decide whether to display a loading indicator? What if the process is known as a quick one but the user is on a slow connection?
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.
Also, if we're extending the height of the input control based on an async request that may take long to process, how do we avoid the content to reflow unexpectedly?
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 shouldn't matter what connection the user is on. This is a detail that would be handled in the implementation.
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.
That's a great question, and I have no idea how we'd avoid reflow in that case.
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.
@mperrotti if there's a server request, wouldn't it make more sense to show the loading spinner regardless? I don't understand how that'd be handled in the implementation 🤔 .
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.
@vdepizzol - I was trying to avoid showing too many loading indicators because they could make the app feel slower. If we wait 1 second, we're still giving the user feedback that something is happening, but only if it's something that's making the user wait a noticeable amount of time.
Does that make sense?
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 think my comment above about grouping error messages could be described in this section instead. The case you mention here is a single error message for a group of controls, but we should document how to present multiple individual error messages for a group of controls as well.
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.
@langermank - if you have an example of multiple individual error messages for a group of controls, that might be helpful.
@alliethu - I remember you saying it's not good to show multiple error messages for a single input, but what about multiple error messages that apply to a group of checkboxes or radios?
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.
Here's a screenshot from a previous form I worked on:
I think the idea here is if there are multiple errors in a single
<form>
, show them in a labeled group with links to each field. My original comment might have sounded like multiple errors per field, but that's not what I mean 😅 that feels problematic in a different way!@alliethu any thoughts here? I pulled this from a book on accessible forms, and that was a year ago. I haven't researched this recently.
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.
@langermank Your annotation is very much inline with what I would have done myself. I'm attaching a rough annotation below that includes a few more details:
Considerations to make:
role="alert"
to the summarydiv
h2
to title the error summary area)div
by usingaria-labelledby
cc/ @mperrotti
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.
Thank you both for the helpful examples! I misunderstood what was being suggested. I thought @langermank was suggesting this pattern for a single group of checkbox or radio inputs.
It feels aggressive to always show the error summary alert banner. We should explain when a summary is needed and when it isn't. Maybe when there are 3 or more errors?
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'm noticing repetitive error messages in @alliethu's example, and that feels like it could be overwhelming or confusing to sighted and non-sighted users.
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 pushed a commit to add some guidance on designing error summaries. I'm not feeling great about it yet, and I'd appreciate some time pairing with somebody from the accessibility team.
Preview deployment