Refine the visual display of the comment reply form help text #1434
Conversation
This PR wraps the help text in and alert, and adjust the spacing between the reply form and the comment above. The goal is to more clearly distinguish the reply form from the comment, and make it clear that the help text is connected to the reply form, not the comment above.| Fixes #1427
🦋 Changeset detectedLatest commit: 359e3bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| {% if wrap_help_text_in_alert %} | ||
| {% embed '@cloudfour/components/alert/alert.twig' %} | ||
| {% block content %} | ||
| {{help_text}} | ||
| {% endblock %} | ||
| {% endembed %} | ||
| {% else %} | ||
| {{help_text}} | ||
| {% endif %} |
There was a problem hiding this comment.
Would this be simpler if we used blocks instead of a property?
| {% if wrap_help_text_in_alert %} | |
| {% embed '@cloudfour/components/alert/alert.twig' %} | |
| {% block content %} | |
| {{help_text}} | |
| {% endblock %} | |
| {% endembed %} | |
| {% else %} | |
| {{help_text}} | |
| {% endif %} | |
| {% block help_text %} | |
| {% if help_text is not empty %} | |
| {% embed '@cloudfour/components/alert/alert.twig' %} | |
| {% block content %} | |
| {{help_text}} | |
| {% endblock %} | |
| {% endembed %} | |
| {% endif %} | |
| {% endblock %} |
There was a problem hiding this comment.
I'm not sure. This component is used in two different places:
- Leaving a comment on a post
- Responding to a comment on a post
I think the following things are the same in both use cases:
- The help text content
- The
aria-describedbylink between the help text and thetextarea
I think the following is different:
- Whether the help text should be wrapped in an alert.
If I'm understanding your suggestion correctly, we would need to pass in the help text content, and maintain the aria-describedby link manually when we didn't want the help text in an alert. That seems like more of a maintenance burden than adding a new property, but I may be misunderstanding the suggestion.
There was a problem hiding this comment.
When do we not want the help text in an alert?
There was a problem hiding this comment.
Ahh, perhaps I had misunderstood. My impression was we didn't want to use the alert in the main "Leave a comment" form.
If that's not the case then you're right that this new prop is over-complicating things. I can switch to using an alert everywhere.
There was a problem hiding this comment.
Yeah, let's keep it consistent!
src/components/comment/comment.scss
Outdated
| /// The top margin matches the grid gap spacing applied to the comment form. | ||
| /// Otherwise, the form's help text appears visually closer to the comment | ||
| /// you're responding to than to the form. | ||
| margin-top: ms.step(2, 1rem); |
There was a problem hiding this comment.
Did you consider using what's used for the replies themselves to inherit any rhythm object spacing?
margin-top: var(--rhythm, #{size.$rhythm-default});
There was a problem hiding this comment.
This is what I was doing previously, but the rhythm spacing between replies is smaller than the spacing within the comment form.
This led to the help text being closer to the comment above it than to the rest of the form. I worried that made it less clear what the help text was connected to.
Though maybe if the rhythm spacing used on the WP site is larger that won't be an issue?
There was a problem hiding this comment.
In that case, can we use whatever token the form is using instead of a new number? (And if it's not using a token, maybe it should be?)
There was a problem hiding this comment.
Sure, I can switch the form to using a token and use it here as well.
src/components/comment/comment.twig
Outdated
| heading_text: "Reply to #{comment.author.name}", | ||
| heading_class: 'u-hidden-visually', | ||
| main_label: 'Reply', | ||
| wrap_help_text_in_alert: true, |
There was a problem hiding this comment.
See earlier comment about using a block instead of a boolean property.
|
Thanks @tylersticka ! Based on your feedback I made a couple changes:
Let me know if you'd like any further changes. Thanks! |
src/components/comment/comment.scss
Outdated
| @use '../../compiled/tokens/scss/size'; | ||
| @use '../../mixins/headings'; | ||
| @use '../../mixins/theme'; | ||
| @use "../../mixins/ms"; |
There was a problem hiding this comment.
Nit: Is this still necessary?
|
@tylersticka removing the |
Overview
This PR wraps the help text in and alert, and adjust the spacing between the reply form and the comment above.
The goal is to more clearly distinguish the reply form from the comment, and make it clear that the help text is connected to the reply form, not the comment above.|
Screenshots
Testing
Fixes #1427
/CC @tylersticka