- Issue created by @mherchel
- Assigned to javi-er
- Merge request !4143Issue #3365625: Migrate Olivero's comments to use single directory components → (Open) created by javi-er
- last update
over 1 year ago Build Successful - Status changed to Needs review
over 1 year ago 6:33pm 8 June 2023 This is ready for review now, however there are some considerations:
- For the component group I used "Content" but let me know if another group should be used instead (or maybe removed)?
- The component is still calling the
olivero/comments
library, because this library is also called fromtemplates/field/field--comment.html.twig
so 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. - The
parent
variable is defined as a string although it's actually aTranslatableMarkup
object, but since it will be empty for the first comment, if it's defined as aTranslatableMarkup
it will throw an error for the first one.
- Status changed to Needs work
over 1 year ago 7:00pm 8 June 2023 Post 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.
- last update
over 1 year ago Build Successful - Status changed to Needs review
over 1 year ago 7:34pm 8 June 2023 This 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 in both places but I'm not sure if it's used on the comments form / wrapper since I can't find it inspecting the page:
.add-comment__picture
.I noticed that Single Directory Components it's not only loading the compiled CSS file but also the PostCSS file, since the extension ends on
.css
as well, I opened an issue regarding this here: https://www.drupal.org/project/sdc/issues/3365730 ✨ SDC is loading Post CSS files, not just compiled CSS files Closed: works as designed- Status changed to Needs work
over 1 year ago 10:12pm 8 June 2023 - Status changed to Needs review
over 1 year ago 2:54am 9 June 2023 @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?
- Status changed to Needs work
over 1 year ago 12:38pm 9 June 2023 - 🇺🇸United States smustgrave
“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.
- 🇺🇸United States mherchel Gainesville, FL, US
I 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.
- 🇺🇸United States mherchel Gainesville, FL, US
Just left some comments in the MR. Lots of good work here. Thanks!
- 🇺🇸United States smustgrave
Since these can’t be merged yet could probably update standard to include sdc? Just to see if the tests pass?
- last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - Status changed to Needs review
over 1 year ago 3:36pm 15 June 2023 I 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.
- Status changed to Needs work
over 1 year ago 4:19pm 15 June 2023 - 🇺🇸United States smustgrave
They'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.
- last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - 🇷🇸Serbia finnsky
What i see that this comment is pretty same except small details(modifiers) to
https://www.drupal.org/project/drupal/issues/3365367#comment-15527524 📌 Convert Olivero's teaser into a single directory component Postponed
So probably we need to try to user one component here.
- First commit to issue fork.
- First commit to issue fork.