Component: Comment #1370

tylersticka · 2021-07-07T17:40:09Z

Overview

Adds a not-quite-complete version of a c-comment component. Remaining to-dos are specified in the "Status" section of the Storybook docs page.

I made a few changes from the designs:

I increased some of the horizontal spacing but tightened up the vertical spacing to make longer threads a bit easier to scan.
I added a 🔗 icon to the permalink for consistency with our permalinked headings.
I did not increase the avatar size to the same degree because I haven't found a technique for doing so yet that I'm happy with. (Do not be surprised if this changes in the next revision!)

Screenshots

Testing

Deploy preview

Remember, you can view a page on its own by opening a story in "Canvas" view and clicking the top right icon that looks like an iOS "share" icon (arrow pointing upward from rectangle). This may streamline VoiceOver testing by focusing on the specific story!

See Add comment thread pattern - dev. #1238

/CC @AriannaChau

changeset-bot · 2021-07-07T17:40:11Z

🦋 Changeset detected

Latest commit: 92142e9

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

tylersticka · 2021-07-07T18:00:44Z

@calebeby I'm having trouble resolving some TypeScript errors related to some imports in a dummy data generator. If I follow what TypeScript tells me to do, I get eslint errors, if I follow what eslint tells me to do, I get TypeScript errors. Help would be appreciated!

tylersticka · 2021-07-07T19:02:13Z

@calebeby Thank you!

tylersticka · 2021-07-07T20:47:23Z

Unfortunately I found a regression, moving this back to draft state until I'm able to address.

Edit: This has been addressed.

gerardo-rodriguez · 2021-07-07T22:51:31Z

This is looking great, @tylersticka!

Quick question…should there be a bit of extra breathing room at the bottom of the reply? It feels uncomfortably close to the bottom edge and makes me think there is more to see/scroll. Of course, this design is out of context and this may not matter when within the full page layout. Figured I'd ask about it. 🙂

gerardo-rodriguez · 2021-07-07T22:54:46Z

src/components/comment/comment.twig

+    <h{{_heading_depth}} class="c-comment__heading">
+      {{comment.author.name}}
+      <span class="u-hidden-visually">
+        {% if comment.is_child %}replied{% else %}said{% endif %}:


I very much enjoyed this audible experience, thank you! ❤️

Paul-Hebert

This looks (and sounds) great!

I left a few nits and questions inline. I think we may want to switch from using a title to visually hidden text for comment links? Everything else is non-blocking though.

Paul-Hebert · 2021-07-07T22:32:13Z

src/components/comment/comment.scss

+  grid-template-areas:
+    'object header'
+    'thread-line content'
+    'thread-line footer';


Oh this is clever. 👍 CSS Grid is awesome

Paul-Hebert · 2021-07-07T22:33:24Z

src/components/comment/comment.scss

+  // We generally want comment contents to align to the start/top, but the
+  // the heading looks better center-aligned with the avatar.


Nit

Suggested change

// We generally want comment contents to align to the start/top, but the

// the heading looks better center-aligned with the avatar.

// We generally want comment contents to align to the start/top, but

// the heading looks better center-aligned with the avatar.

@Paul-Hebert I've addressed this feedback, but I removed a different "the" because there was plenty of room below the 80 character threshold for that line.

Paul-Hebert · 2021-07-07T22:47:12Z

src/components/comment/comment.twig

+      src: comment.avatar,
+      size: 'full'
+    } only %}
+  </div>


Are we confident there will always be avatars when using this pattern? (e.g. will we use Wordpress's fallbacks or something else if there's no avatar?)

Yes, and if the avatar is omitted, you'll just see a placeholder circle with the current pattern, so it's pretty fault-tolerant.

Paul-Hebert · 2021-07-07T22:50:03Z

src/components/comment/comment.twig

+      <div class="c-comment__meta-detail">
+        <a class="c-comment__meta-link"
+          href="{{comment.link}}"
+          title="Permalink to {{comment.author.name}}’s {{comment.date|date('M j')}} {% if comment.is_child %}reply{% else %}comment{% endif %}">


My understanding is that some screen readers have issues with the title attribute: https://www.tpgi.com/using-the-html-title-attribute

I wonder if it would be better to include some visually hidden text for assistive technologies?

I tried this in VoiceOver and the title text wasn't announced so I just heard a link with a date. It seems like this title info would be useful context for assistive technologies.

Thanks, @Paul-Hebert! I saw the title attribute usage and was trying to find a reference article but you already found one. I wasn't 100% sure if using title might be cause for concern but I had stored somewhere in the back of my mind that I should be cautious.

FWIW, I did hear the info in the title attribute using VO + Safari + keyboard as my input (which I was surprised by) but the fact we heard varying, inconsistent experiences is reason enough to use visually hidden text instead. 😉

Thanks again, @tylersticka, for the thoughtful UX design! ❤️

Oh, interesting! I wonder why we heard different things 🤔

@Paul-Hebert @gerardo-rodriguez I believe I've addressed this comment by moving the text into visually hidden span elements. One nice side effect of this is that I can now re-use the existing time element for that portion of the fallback text.

I now get the full context, thanks!

Paul-Hebert · 2021-07-07T22:51:05Z

src/components/comment/comment.twig

+          via <a class="c-comment__meta-link" href="{{source.url}}">{{source.name}}</a>
+        </div>
+      {% endif %}
+      {% if demo_control %}


Nit: could we add a comment explaining this var name? It confused me until I went back to the docs page

@Paul-Hebert Good idea, I've added a TODO comment with more context.

tylersticka · 2021-07-07T23:06:30Z

Of course, this design is out of context and this may not matter when within the full page layout. Figured I'd ask about it. 🙂

@gerardo-rodriguez We use a Storybook add-on called addon-paddings that did an update recently that seems to have added some regressions in the padding of displayed elements. Another example occurs if you look at the Button docs at any stories that are side by side, you'll see the buttons now run together. The good news is that it doesn't have anything to do with my comment CSS, it's something you can recreate in other stories prior to this PR.

Would greatly appreciate a separate issue to address!

gerardo-rodriguez · 2021-07-07T23:38:39Z

src/components/comment/comment.twig

+      <h{{_section_heading_depth}} class="u-hidden-visually">
+        Replies
+      </h{{_section_heading_depth}}>


Clever! I totally missed this the first time I turned VoiceOver on.

For a moment, I had a slight concern that seeing multiple "Replies" headings might be confusing within the context of the VO rotor but since the rotor lists them in order, I think there is enough contextual information that it makes sense. 👍

@gerardo-rodriguez I've made a slight adjustment based on this observation, adding the original comment author's name to the "Replies" heading. I think this makes the headings more distinct and the relationship a bit more clear between heading levels now.

This seems like a great improvement 👍

Paul-Hebert

Looks great! 🚢

dejavu1987 · 2022-12-22T17:49:30Z

src/components/comment/demo/data.ts

-const random = require('lodash/random');
-const sample = require('lodash/sample');
-const uniqueId = require('lodash/uniqueId');
+import Jabber from 'jabber';


Hi @tylersticka , thanks for using Jabber, I have added ts support in v.1.5.2.
regards
Anil

dejavu1987 · 2022-12-22T17:52:48Z

Hi @tylersticka , thanks for using Jabber, I have added ts support in v.1.5.2.
regards
Anil

tylersticka added 12 commits July 7, 2021 09:05

WIP comment markup

d1af23d

WIP comment styles

197119c

More CSS progress

0924c58

Better comment data

1ed46d7

Properly theme comment colors

b01e103

Content-complete meta section

f07aaa9

Add size.edge.control token

daca518

Add reply button (WIP)

e0a347d

Adjust content negative margin

4f44687

Clean up dependencies

40ac936

Add comments to comment (heh)

66dc75c

Add changeset

fdd450a

tylersticka added 2 commits July 7, 2021 10:44

Fix Typescript complaints

55554f1

Fix some TypeScript errors (but not all)

fa5d5c8

Fix remaining TS errors

798db61

tylersticka requested review from a team July 7, 2021 19:03

tylersticka marked this pull request as ready for review July 7, 2021 19:03

tylersticka removed request for a team July 7, 2021 20:46

tylersticka marked this pull request as draft July 7, 2021 20:47

Improve comment.children check

540a9fc

tylersticka marked this pull request as ready for review July 7, 2021 21:15

tylersticka requested review from a team July 7, 2021 21:16

gerardo-rodriguez reviewed Jul 7, 2021

View reviewed changes

Paul-Hebert reviewed Jul 7, 2021

View reviewed changes

gerardo-rodriguez mentioned this pull request Jul 7, 2021

Fix Storybook padding regressions #1371

Closed

gerardo-rodriguez reviewed Jul 7, 2021

View reviewed changes

tylersticka added 4 commits July 8, 2021 08:39

Correct CSS comment typo

be8da09

Move permalink fallback text from attr to content

7f0d1a7

Make replies section headings more distinct

0148588

Add comment to temporary demo_control prop

92142e9

tylersticka requested review from a team July 8, 2021 15:52

Paul-Hebert self-requested a review July 8, 2021 15:57

Paul-Hebert approved these changes Jul 8, 2021

View reviewed changes

tylersticka merged commit 157fad1 into v-next Jul 8, 2021

tylersticka deleted the feature/c-comment branch July 8, 2021 15:59

github-actions bot mentioned this pull request Jul 8, 2021

Publish Next Version #1354

Merged

tylersticka mentioned this pull request Jul 8, 2021

Replace Message with Alert, use in Comment #1373

Merged

dejavu1987 reviewed Dec 22, 2022

View reviewed changes

		// We generally want comment contents to align to the start/top, but the
		// the heading looks better center-aligned with the avatar.

Component: Comment #1370

Component: Comment #1370

Uh oh!

Conversation

tylersticka commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Screenshots

Testing

Uh oh!

changeset-bot bot commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

tylersticka commented Jul 7, 2021

Uh oh!

tylersticka commented Jul 7, 2021

Uh oh!

tylersticka commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gerardo-rodriguez commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Paul-Hebert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerardo-rodriguez Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylersticka commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Paul-Hebert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dejavu1987 commented Dec 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tylersticka commented Jul 7, 2021 •

edited

Loading

changeset-bot bot commented Jul 7, 2021 •

edited

Loading

tylersticka commented Jul 7, 2021 •

edited

Loading

gerardo-rodriguez Jul 7, 2021 •

edited

Loading