-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Use labels instead of placeholders on contact form #2050
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
Use labels instead of placeholders on contact form #2050
Conversation
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.
Wow, thanks for taking the time to submit this! 🙏🏻
I think the changes to move away from placeholder
in favor of a real <label>
are the way to go. I've left a suggestion for a simplification of the template.
As for the arrow in the send button, I remember @marksweb and me discussing it on his PR to make the site translatable and I was the one to suggest including the →
into the translatable string, on the basis that some languages might want to invert the direction. But that's not something I'm very confident about so I'd be happy to be told that's not a good practice. Also, even though most of the site is marked for translation we currently don't have any infrastructure for it, so don't be afraid to make changes since it won't actually break anything in that regard.
Thanks again for sending this, and don't hesitate to open other accessibility-improvements PR! 🎸
<form action="." method="post" accept-charset="utf-8" class="form-input"> | ||
{% csrf_token %} | ||
<p> | ||
<label for="{{ form.name.id_for_label }}">{{ form.name.label }}</label> |
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 could use:
<label for="{{ form.name.id_for_label }}">{{ form.name.label }}</label> | |
{{ form.name.label_tag }} |
It's not strictly equivalent since it will add a :
after the label but I think that's not a bad change.
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.
Oh I like this!
</p> | ||
<p class="form-email"> | ||
<label for="{{ form.email.id_for_label }}">{{ form.email.label }}</label> | ||
{% if form.email.errors %}<p class="errors">{{ form.email.errors.as_text }}</p>{% endif %} |
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.
Oh I've seen something else that needs a fix: if form.email.errors is true, then you will have two nested <p>
s, which will break something. I will fix it in this PR too.
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.
Ah yes, good catch! If we're ok using a <div>
to wrap up the label+input+errors we could simplify the template a lot and use:
<div>{{ form.name.as_field_group }}</div>
55a3c73
to
6936b83
Compare
I'll come back to this later (this week I hope?) to fix this nested |
For the send button, we could add the arrow in CSS generated content (like ::after) with an empty alternative text; then use (Oh and to use ::after we'll need to change the input submit into a button.) |
4bbe83d
to
262847b
Compare
I tried to add the css-generated arrow in the "send" button, and then… things happened :D |
<a href="https://forum.djangoproject.com">Django Forum</a>. | ||
{% endblocktranslate %} | ||
</p> | ||
<form action="." method="post" accept-charset="utf-8" class="form-input"> |
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 removed this class "form-input" as it made the <button>
move on top right of the form, apparently because of an ancient class (2014) not used anymore for its original purpose throughout the site.
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.
an ancient class (2014) not used anymore for its original purpose throughout the site.
That's probably a good 20% of our CSS styles at this point 😭
d2c5a4e
to
23a414c
Compare
This is my final proposal: no update on error colors, I would like to compare with other colors through the website (currently it's not perfect but it was worse when the errors were not highlighted at all). I added the styling with CSS-generated arrow on the "send" button, which adapts when the language is right-to-left. |
I like the CSS changes to remove the |
djangoproject/scss/_style.scss
Outdated
.cta.arrow { | ||
height: auto; | ||
&::after { | ||
content: " →" / ""; |
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 this is cool, I didn't know about the /
syntax for content
🤯
Yes, go for it 👍 |
23a414c
to
262847b
Compare
I've extracted the 3rd commit into a dedicated PR @bmispelon so you can merge more easily. ❤️ |
I agree on a separate PR for moving arrows into css. It will make things much cleaner 👏👍 |
This PR to add the bare minimum of accessibility on the form to contact the DSF.
Basically, display labels in dedicated
label
s instead of using theplaceholder
attribute.That helps:
When I'll have the whole "translation" part figured out I will try to fix the send button label (
"Send →"
). Ideally I would like to have the→
in an span with aria-hidden=true because it is decorative.