We need to refactor our comments JavaScript

  • Wrap it in a Drupal behavior
  • Use data attributes as selectors where possible

https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/themes/olive...

CommentFileSizeAuthor
#21 3208372-21.patch7.53 KBpaulocs
#21 interdiff-20-21.txt1.18 KBpaulocs
#20 3208372-20.patch7.87 KBpaulocs
#19 3208372-19.patch7.84 KBpaulocs
#19 interdiff-8-19.txt1.73 KBpaulocs
#8 3208372-8.patch7.22 KBmherchel
#6 interdiff-2-6.txt1.31 KBmherchel
#6 3208372-6.patch7.05 KBmherchel
#3 interdiff_2-3.txt11.27 KBgauravvvv
#3 3208372-3.patch5.54 KBgauravvvv
#2 3208372.patch7.09 KBmherchel

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Active » Needs review
StatusFileSize
new 3208372.patch7.09 KB

Note that I wasn't able to modify a template to insert a data attribute for the .indented selector. This markup is generated within the comment module at https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/modules/comm...

This should be good to go!

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3208372-3.patch5.54 KB
new interdiff_2-3.txt11.27 KB

I have provided the patch please review.

Status: Needs review » Needs work

The last submitted patch, 3: 3208372-3.patch, failed testing. View results

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3208372-6.patch7.05 KB
new interdiff-2-6.txt1.31 KB

The interdiff in #4 didn't match the patch. Either way, we don't want to introduce new node dependencies.

The reason #2 failed is because babel (on the D.O servers) didn't understand the optional chaining syntax. I'm removing that in this patch.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3208372-8.patch7.22 KB

Attaching a new patch that uses the [data-drupal-selector] pattern.

Tugboat preview: https://3208372-refactor-comments-js-py9veecd6lb02zgxukvwkzknubwmvwtv.tu...

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented 
--- a/core/themes/olivero/templates/field/field--comment.html.twig
+++ b/core/themes/olivero/templates/field/field--comment.html.twig
@@ -29,7 +29,7 @@

-<section{{ attributes.addClass('comments') }}>
+<section{{ attributes.setAttribute('data-drupal-selector', 'comments').addClass('comments') }}>

I don't think we're using the class name 'comments' anywhere. Do we still want/need to attach that class since it's not doing anything?

Besides that, I think we're good

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Yeah, I say we should still include it. Eventually the starterkit theme generator will be able to copy this theme, and in that case the class might be useful for people to style against.

thejimbirch’s picture

thejimbirch commented

If that is the case, then RTBC.

thejimbirch’s picture

thejimbirch commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 3208372-8.patch, failed testing. View results

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Reviewed & tested by the community

Unrelated failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 3208372-8.patch, failed testing. View results

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Reviewed & tested by the community

Unrelated failure.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/olivero/js/comments.es6.js
@@ -1,39 +1,49 @@
+  function init(comments) {
...
+  Drupal.behaviors.comments = {

These should be documented according to the coding standards https://www.drupal.org/docs/develop/standards/javascript/javascript-api-...

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

Working on it.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-8-19.txt1.73 KB
new 3208372-19.patch7.84 KB

Attaching patch and interdiff.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
StatusFileSize
new 3208372-20.patch7.87 KB

Sorry for the patches. I'm starting now contributing to front-end. I'm having problems with prettier.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
StatusFileSize
new interdiff-20-21.txt1.18 KB
new 3208372-21.patch7.53 KB

I think now it is right...

marcusvsouza’s picture

marcusvsouza commented
Status: Needs review » Reviewed & tested by the community

RTBC!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

+1 RTBC

  • lauriii committed 630cd36 on 9.3.x 
    Issue #3208372 by paulocs, mherchel, Gauravmahlawat: Olivero: Refactor...

  • lauriii committed 60d7c69 on 9.2.x 
    Issue #3208372 by paulocs, mherchel, Gauravmahlawat: Olivero: Refactor...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Noticed the use of indented class but since it's in a piece of markup that is hard to override, I decided to open issue in the comment module issue queue instead: #3221204: Move markup to templates from CommentViewBuilder.

Committed 630cd36 and pushed to 9.3.x. Also cherry-picked to 9.2.x since Olivero is experimental. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.