Remove non-existent dependency core/collapse in claro.libraries.yml

Just providing sweetchuck's idea as a patch.

Fair point.

I was running into a bug where the node tabs in desktop safari were collapsing themselves (the way they do on a mobile device). And...it's only happening on one machine and nobody else's. Digging through the issue queue this seemed like maybe a potential cause so I wanted to get a patch to test it out. Turns out this didn't fix *that* problem.

The code was introduced in #665790: Redesign the status report page and some subsequent work in #2854624: Details and accordion update based on Seven Style Guide also seems to leverage collapse.js from core.

Looking at docroot/core/themes/stable/templates/admin/status-report-grouped.html.twig it is attaching the library (as are several other files).

I think the dependency is real. I'm not sure why it hasn't been noticed.

Patch Failed

I have applied the patch #13 cleanly on drupal 9.3.x-dev and the core/collapse library dependency is removed from seven.libraries.yml. Now it is replaced with proper dependency core/drupal.collapse.
Adding screenshot for reference.

rerolling for Drupal 9 and I noticed that also affects claro so I made the change there as well. Also moving over there so they are aware.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rerolled the #15 updated the Non-exists dependency in claro theme for D10.1.x.

Simple enough fix.

This issue has been open for five years. Do we actually have a bug in Claro that's been there for five years, or does the library not really need this dependency at all? Marking for manual testing. I don't think we need automated test coverage here because it's just a one line change, but would be good to know if there's a real problem that this fixes.

Also think we should look at issuing assert() errors when library definitions have missing dependencies.

Feel like there was a ticket for checking for missing libraries but can't find it right now.

Think you are right if this hasn't caused an issue in 5 years then it's a library not needed by claro and should just be removed.

Yes. That sounds good to me to remove it and keep the assert to a followup.

I tried to create this as an MR.

Thanks @greggles!

Tagging for subsystem maintainer review.

Re #23 and #24 there is #2747339: Assert if a library definition is not valid / as expected which doesn't exactly cover this issue but could be expanded to do so, I think.

So nav tabs don't use details so the dependency is wrong

And I could not find any code using/including the drupal.responsive-detail library, so no harm removing the dependency.

I think that fix is fine for D10, but for D9 wasn't it required as a polyfill for the details element for IE11?

Still has the needs subsystem maintainer review tag, so will ping in slack

change title to indicate what this is fixing.

I don't think it's valuable to spend more time on it here, committing as is and added a followup #3353612: Remove or fix claro/drupal.responsive-detail to figure out the actual place it may be useful. nav tabs definitely don't need this dependency.

As far as backport goes, since it doesn't look like anyone really noticed I say leave it as-is?

Removing seven from the title since it's only claro now :) (but the commit message went out with seven…)

Committed dc0c894 and pushed to 10.1.x. Thanks!