Theme/Config Comments

Hi, I'm taking this on. Aiming to have something reviewable by end of this weekend (3/29).

FYI, we just merged #3117242: Theme/style the Form elements (unsure if you were blocked)

Update here, still working on things. Free time hasn't been as abundant as I thought, but there's a good bit done here on my end!

I'll get as much "MVP" stuff wrapped up tonight, then propose some followups.

OK, this patch is ready for a review. I pretty much just created an Article, and started making comments to test/theme this.

Some notes/questions:

• Comment form not quite matching comp, it didn't account for WYSIWYG or a subject field in the comp, should it?
• The comp doesn't account for comment "subject" text (currently an h3) - I left it in anyway.
• Be sure to look at comment reply form/pages, like /comment/reply/node/1/comment/2
• Should we have square-cropped user pictures expected/configed? That's what we'd need to accomplish the circular user picture approach here. Otherwise, I guess we'll have to do some kind of fancier CSS "masking" approach to only show a portion of a non-square image.

Some things I didn't touch or had trouble with, maybe they are followups (if we want to get this merged in ASAP)?

• The timestamp on the comments - couldn't quite get it in just a "days ago" format, had to settle for days/hours due to time
• The "Show/Hide Replies" toggling suggested in the comp, for nested comments
• The vertical line connecting the user pictures on nexted comments
• The "new" marker that shows up after a new reply is posted - I didn't style it because it wasn't comped
• Missing a user picture to the left of the comment form.

Re-rolled the patch to fix a few things I saw after the fact. And attaching a screenshot of what I'm seeing locally (for reference).

Thanks for your work @jerseycheese, I'm gonna have a look into your patch!

Mmmm.. i didnt have free time to check this task today, maybe tomorrow)

This is starting to come together!

Some visual critiques:

1. +++ b/css/src/components/comments.css
   @@ -0,0 +1,204 @@
   +  display: flex;
   

   This is super nitpicky, but you'll want to put your display properties first (see https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...). Note that we really haven't been following coding standards very good yet, but this'll save us some cleanup in the future.

2. +++ b/olivero.libraries.yml
   @@ -18,6 +18,7 @@ global-styling:
   +      css/dist/components/comments.css: {}
   

   Let's create a separate library called "comments" and use {{ attach_library() }} to load the library when the comment template is loaded. This will make a smaller CSS bundle.

3. +++ b/templates/node.html.twig
   @@ -68,6 +68,7 @@
   +{#{{ vardumper(content) }}#}
   

   Debug code

Overall, this is starting to come together!

Note I'll come in at some point and adjust those negative margins so they conform to the grid exactly... but they're super close now.

Thanks for all the work @jerseycheese!

Reroll was done.

I didnt work on that issue too much, since not enough free time today for that on my side, but what was done:
1. Cleanup of generated css. It was about:

+
+/*# sourceMappingURL=grid.css.map */

2. Added user_picture to field--comment.html.twig (without styles, only implemented from function olivero_preprocess_field__comment(&$variables) { }

For now that's all

Btw don't forget to check comments in IE11.

Fully ready for review.

1. Vertical bar added
2. "Hide replies" button added
3. Post comment button added
4. Added default gray circle if no avatar uplodaded
5. Fixed mobile styles

I also think we have to create a follow-up issue for Drupal 9 about creation of the new image style (square, scale & crop, maybe around 100x100 pixels or so) and new user view display with attached new image style for "Standard" profile.

This is starting to look really good!

Some code comments below, plus some visual issues coming up:

1. +++ b/js/comments.es6.js
   @@ -0,0 +1,42 @@
   +    const replyBtn = document.createElement("button");
   

   Need to use single quotes per Drupal coding standards.

2. +++ b/js/comments.es6.js
   @@ -0,0 +1,42 @@
   +    replyBtn.addEventListener("click", button => {
   

   The addEventListener method passes the event object, rather than the target of the event., so it'll need to be `e` and then button would be `e.currentTarget`

I'm attaching an updated patch that starts to handle all of the indentation, and also fixes the JS errors.

That being said, the vertical line still doesn't always show properly. Still needs a bit of work.

current status:

@mherchel Hello! Thanks for your feedbacks, your help and fixes. Can you please provide interdiff next time? Since it's hard to understand what was changed :)

I tried to fix all possible cases i found, then i tried to test comments as much as i could and it looks good on my side now. A couple important points:
- these lines are currently sensitive to js and styles based on js. If js is disabled in browser - you will not see these side lines.
- 3rd level comments and deeper are "linked" to the 2nd level comments visually. Check my screens.
- mobile styles were adapted.

Also let's not lint js in comments here. We need a task to add eslint.js and eslintrc.passing.json files in theme ! Since we have yarn scripts to lint, but it doesn't work currently (i mean yarn lint:js-passing or yarn lint:js commands are not working properly.

1.

2.

3.

Wow! This is looking so good :) I'll be able to look at it in-depth today (going to start now, but not sure I have enough time this am)

I'm also thinking - maybe it's better to keep replies hidden by default?

Let's leave them expanded for now, and float the idea. It'd be an easy enough change to make later.

First pass is looking pretty good. Only requested changes are some naming. I'll take a closer look at things later today, also.

1. +++ b/js/comments.es6.js
   @@ -0,0 +1,64 @@
   +    const replyBtn = document.createElement('button');
   

   This name is kind of confusing. How bout showHideBtn?

2. +++ b/js/comments.es6.js
   @@ -0,0 +1,64 @@
   +    replyBtn.setAttribute('class', 'reply-btn');
   

   Change the CSS classname too please :)

3. +++ b/js/comments.es6.js
   @@ -0,0 +1,64 @@
   +          ? Drupal.t('Show Replies')
   

   I'm fairly confident that the text presented to the accessibility tree shouldn't be modified on click. Need to double check with accessibility folks to be sure.

Tested in IE11, and everything is looking good.. there's some grid issues with the article content type, but that's external to this issue.

User image needs to be aligned to the input text:

Let's indent the second level comments at mobile. We'll have to indent the third level comments even more... but I'm okay with that.

I love the treatment when there is no user image ❤️

Some accessibility comments from https://drupal.slack.com/archives/C2ANFUGGG/p1586184306065500:

Get rid of the aria-label attribute entirely. You don't need it, because there is visible text.

Don't change the name of the button. Use a disclosure pattern instead. Keep the name of the button constant. Convey the state using aria-expanded and visual styling, such as a +/- icon.

This is a button, right? You might consider a checkbox or role=switch. Same logic though: constant visible name, and dynamic checked property. Can be styled visually as a flip-switch or whatever. Personally I think button/expanded us better than checkbox/che ked for this. (edited)

Does the .hidden class provide either display:none or visibility:hidden? If so, you don't need the aria-hidden attribute

All reported feedbacks should be fixed now.
About buttons - i went with youtube way. Now we have 2 different buttons, one of them is hidden until 2nd one will be clicked:

<div class="show-hide-wrapper">
  <button type="button" aria-expanded="false" class="show-hide-btn show-hide-btn--show hidden">Show Replies</button>
  <button type="button" aria-expanded="true" class="show-hide-btn show-hide-btn--hide">Hide Replies</button>
</div>

Swapping in and out the buttons won't work because 1) there's no indication that's going to happen (which can confuse people), and 2) Focus state between the buttons isn't managed.

If we can't change the text, then we should just use a +/- indicators.

I'm attaching an updated patch (and interdiff!). I'm ready to commit this if it's good. Note I also plan on changing the order of the CSS properties, but didn't put them in the patch for clarity.

According to https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...

1. Positioning properties include: position, float, clear, top, right, bottom, left, direction, and z-index.
2. Box model properties include: display, [(max|min)-]height, [(max|min)-]width, margin, padding, border and their various longhand forms (margin-top, etc.) Plus box-sizing.
3. Other declarations.

Forgot to attach patches!

Hello, thanks for your interdiff :)

Honestly i don't like at all how it looks now.. text is overthinked from my point of view. Too much visual information just for a small button. But i'm not accessibility expert for this case, so i maybe wrong and it is normal situation to have lots of text here.

You told

1) there's no indication that's going to happen (which can confuse people)
2) Focus state between the buttons isn't managed.

So it's not a problem to add indication for both buttons. And is it really problem about focus states? (Idk really)

Also maybe we can redesign this button?

Anyway i think it can be merged and follow-up task created.

Also i dont think you have to take care about css order, we need to fix yarn lint commands before

Committed!

I looked at the JS which was committed - it looks slick, and doesn't over-use ARIA. Refreshingly simple, compared to some other disclosure buttons I've seen. I like the way that the value of aria-expanded is determined by the actual classList itself, so the styling class and state semantics are always aligned.

Re. #32-33 - managing 2 buttons would certainly have added extra complexity, with managing focus as one button disappears. Sticking to one button was the right call.

Regarding the name of the button. The problems with changing the button name (show-replies <-> hide-replies) are:

• It conflates the name (purpose) of the control with it's current state. You'd have "hide" combined with "expanded". The name is the opposite of the state, and a screen reader user hears both words. There's a risk they will mix up which is the name and which is the state.
• Controls are easier to find if they keep the same name throughout. Particularly for screen reader users or magnifier users. e.g. if you change your mind, and go back to find the button again to close the group. If the name has changed it can be trickier.

The first problem is described by Leonie Watson in this demonstration webinar:

Toggle buttons are really hard. Play/pause buttons are the notorious example. I hit "play" and then the button appears to be pressed, and then something starts playing. But then the label changes to "pause" and I'm never quite sure whether the pause button means if I hit it it will pause, or whether it's already paused and if I hit it, it will go back to play. Something like aria-pressed is a good way of saying keep the label the same, but indicating along with the visual design that this thing is pressed or not-pressed.

Source: How A Screen Reader User Accesses The Web: A Smashing Video. There isn't a transcript sadly, but this came from the 54m23s mark.

Here's an interesting article (and recent research) from Sarah Higley: Playing with State. Key points:

• It discusses the name vs. state difference a bit more. The state being talked about there is aria-pressed, rather than aria-expanded, but the principle is the same.
• It also has some research about the current state of browser/screen-reader combinations. They don't all convey a change of button name to the user! That's the crucial consideration, I think. OTOH the aria-expanded and aria-pressed states are communicated reliably in every browser/screen-reader combination I can think of.
• She disagrees slightly with Leonie, and recommends that a name change is OK for the play/pause scenario, just because it's so well-known. However she recommends constant name + dynamic state for all other cases.

Overall, I think that "Show/hide replies" is too long for the button name. I'd go for "Show replies" followed by the open/closed state.

(I sometimes think it helps to think of the name of the control as a question, rather than an action. So here it would be something like "Show replies? [Yes/No]". I don't mean it should literally have a question mark; that's just how I think about it.)

Another way to phrase it might be "View replies [+/-]", or even just "replies [+/-]".

Regarding the visual symbol which shows the state. We already have the plus-minus in the narrow-screen sub-menus. But the wide-screen menus use a drop-arrow. It might be worth revisiting this, to decide on one or the other for consistency. The difference is that one involves swapping a symbol, the other involves rotation. When using plus-minus you have the visual symbol being the opposite of the state . A triangle/arrow does it with direction change instead of a symbol change (e.g. points right when closed, points down when open). I think that's why the triangle is more common nowadays.

Adding the accessibility tag for posterity. I might do a round up of these in a blog article once we're stable.