Skip to content
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

Integrate reply Comment Form into Comment pattern #1423

Merged
merged 49 commits into from
Jul 22, 2021

Conversation

Paul-Hebert
Copy link
Member

@Paul-Hebert Paul-Hebert commented Jul 20, 2021

Overview

The primary focus of this PR is integrating the Comment Form for replies into the Comment pattern.

While I was working on this I made some changes to the Comment Form component and updated the code examples for all the Comment stories.

I could use both design + dev review on this.

Follow-up issues will handle:

  • finalizing template properties
  • handling form submission/validation

Screenshots

reply-pattern

Testing

  1. Review the Comment docs page for regressions
  2. Review the Comment Form docs page for regressions
  3. Review the Reply experience. (Also try toggling the isLoggedIn and allowReplies controls
  4. Review a more complex Reply experience using VoiceOver and make sure the screen reader experience is clear and intuitive

@changeset-bot
Copy link

changeset-bot bot commented Jul 20, 2021

🦋 Changeset detected

Latest commit: cf021e7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudfour/patterns Minor

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

- `heading_id`: A unique ID for your heading element. This will be used as the accessible name for the form.
- `heading_tag`: The tag for your heading (defaults to `h2`).
- `heading_text`: The text for your heading.
- `heading_class`: An optional class for your heading.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was tempted to replace this all with a heading block, but that makes it more likely for the aria-labelledby relationship between the form and heading to get messed up.

@Paul-Hebert Paul-Hebert changed the title Feat/integrate reply form Integrate reply Comment Form into Comment pattern Jul 20, 2021
docs: {
source: {
code: makeTwigInclude(
'@cloudfour/components/comment-form/comment-form.twig',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here? 🤔

Suggested change
'@cloudfour/components/comment-form/comment-form.twig',
'@cloudfour/components/comment/comment.twig',

docs: {
source: {
code: makeTwigInclude(
'@cloudfour/components/comment-form/comment-form.twig',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too? 🤔

Suggested change
'@cloudfour/components/comment-form/comment-form.twig',
'@cloudfour/components/comment/comment.twig',

docs: {
source: {
code: makeTwigInclude(
'@cloudfour/components/comment-form/comment-form.twig',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'@cloudfour/components/comment-form/comment-form.twig',
'@cloudfour/components/comment/comment.twig',

docs: {
source: {
code: makeTwigInclude(
'@cloudfour/components/comment-form/comment-form.twig',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'@cloudfour/components/comment-form/comment-form.twig',
'@cloudfour/components/comment/comment.twig',

docs: {
source: {
code: makeTwigInclude(
'@cloudfour/components/comment-form/comment-form.twig',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'@cloudfour/components/comment-form/comment-form.twig',
'@cloudfour/components/comment/comment.twig',

@Paul-Hebert
Copy link
Member Author

The latest round of feedback has been addressed. Re-review appreciated, thanks!

Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works much better on small screens! Thanks, @Paul-Hebert! Deferring further design feedback to the separate issues created and deferring approval to @cloudfour/dev since they had more immediate feedback to resolve.

@calebeby
Copy link
Member

@Paul-Hebert It looks like this comment is not resolved?

@Paul-Hebert
Copy link
Member Author

@Paul-Hebert It looks like this comment is not resolved?

Ack, thanks @calebeby , fixed!

calebeby
calebeby previously approved these changes Jul 22, 2021
Copy link
Member

@calebeby calebeby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and tests look great! I didn't closely test it, but I assume that Tyler and Gerardo did

(once lint check is fixed)

@Paul-Hebert
Copy link
Member Author

Oh no! Since we updated Eslint the lint rules are different in the CI task from what I've got locally. One moment while I resolve this...

@Paul-Hebert Paul-Hebert merged commit 9b34589 into v-next Jul 22, 2021
@Paul-Hebert Paul-Hebert deleted the feat/integrate-reply-form branch July 22, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comment form reply behavior
4 participants