Needs work
Project:
Drupal core
Version:
main
Component:
Olivero theme
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
8 Jun 2023 at 13:47 UTC
Updated:
9 Jul 2024 at 07:57 UTC
Jump to comment: Most recent
Olivero's comments should be using SDC. The file to start with is at core/themes/olivero/templates/content/comment.html.twig

| Comment | File | Size | Author |
|---|---|---|---|
| Drupal_9_Olivero__Turning_Conversation_into_a_Core_Initiative___Olivero.png | 204.95 KB | mherchel |
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
javi-er commentedComment #4
javi-er commentedThis is ready for review now, however there are some considerations:
The component is still calling theolivero/commentslibrary, because this library is also called fromtemplates/field/field--comment.html.twigso both, the styles and JS code should be split into the lines affecting specific comments and the comments wrapper and also because I'm not sure if post css preprocessing is set up for components folder, but let me know if this should be changed.parentvariable is defined as a string although it's actually aTranslatableMarkupobject, but since it will be empty for the first comment, if it's defined as aTranslatableMarkupit will throw an error for the first one.Comment #5
javi-er commentedComment #6
javi-er commentedPost CSS will compile css found in components (since it's looking for everything inside the `core` folder), will move styles and move back to needs review.
Comment #7
javi-er commentedThis ready for review now, I'm interested in knowing if the approach I took for splitting the styles that affects the comments wrapper and the individual comments is the right one.
There is one selector that's using duplicated styles for rendering the user picture:
.add-comment__picture. However I couldn't find this selector on the comment form, it seems like that the variableuser_pictureatolivero/templates/field/field--comment.html.twigis empty, even if the user has a picture. In any case, I would like to know the right approach to take when the same styles are used in two places, it could use a separate component just for the user picture or the same class in both places.Comment #8
smustgrave commentedSeems there were some failures in the MR.
Comment #9
javi-er commented@smustgrave the MR is green, the pipeline tab in it is empty and the CI job is building successfully: https://www.drupal.org/pift-ci-job/2687312
Could you specify which problems are you seeing?
Comment #10
smustgrave commented“Build successful” with a grey box means tests did not pass. Usually a night watch test I think.
Would check here https://dispatcher.drupalci.org/job/drupal_patches/186199/console
When the bar is green and it says xyz tests passed then it’s good.
Comment #11
mherchelI don't know if this will ever pass tests. The SDC is not enabled by default within the standard profile, so when it goes to test the comments, it's going to get a WSOD.
Comment #12
mherchelJust left some comments in the MR. Lots of good work here. Thanks!
Comment #13
smustgrave commentedSince these can’t be merged yet could probably update standard to include sdc? Just to see if the tests pass?
Comment #14
javi-er commentedI did the suggested changes and some cleanup, however tests are still failing. I'm moving it to needs review to test the functionality, and the failing test can be addressed once SDC is part of core.
Comment #15
smustgrave commentedThey're failing because SDC is not part of the standard profile install. You can add the module and push up here (others have done that) and see if the tests pass. But the SDC can't be added to standard until it's marked stable.
These olivero tickets are just getting a leg up on when that happens. So SDC will be marked stable, added to standard, then all these tickets should be good to go with (hopefully) minimal tweaking if at all.
Comment #16
finnsky commentedWhat i see that this comment is pretty same except small details(modifiers) to
https://www.drupal.org/project/drupal/issues/3365367#comment-15527524
So probably we need to try to use one component here. And move that collapse button to separated component.