Problem/Motivation
One of the most awesome benefits of adding vertical tabs in Drupal 7 was the javascript that allowed people to see what the current values were without having to pop-open the fieldsets. This benefit has been lost by moving the tabs to the right hand column.
I did a little research into the development of this new display of vertical tabs, and it looks like the current values are supposed to be present - at least according to the screenshots shown in this issue - the one that got committed.
Perhaps they were forgotten, or removed later for some other reason?
Proposed resolution
Put back the awesomeness that was vertical tabs - javascript revealing current values from within fieldsets. In Drupal 7.38 it's handled in /misc/collapse.js, line 56-101, propably doesn't work one-by-one to D8.
Remaining tasks
see above
User interface changes
see above
API changes
none
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#127 | 1936708-127.patch | 22.6 KB | bnjmnm |
#127 | interdiff_123-127.txt | 2.11 KB | bnjmnm |
#126 | 1936708-126.patch | 22.63 KB | ayushmishra206 |
#126 | interdiff_123-126.txt | 1.24 KB | ayushmishra206 |
#123 | 1936708-123.patch | 22.63 KB | bnjmnm |
Comments
Comment #0.0
jenlamptontypo
Comment #0.1
yoroy CreditAttribution: yoroy commentedfixed img tag: srtc=" -> src="
Comment #0.2
yoroy CreditAttribution: yoroy commentedasd
Comment #1
yoroy CreditAttribution: yoroy commentedNot sure why that doesn't work. Here's the link to the image:
http://drupal.org/files/Screen%20Shot%202012-10-13%20at%2012.45.57%20AM.png
Comment #1.0
yoroy CreditAttribution: yoroy commentedstill trying to fix img tag
Comment #2
rootworkThis appears to still be the case in 8.0.x dev, and seems like a real shame. What was apparently intended, as Jen points out and as is shown in the image, would make a lot of sense, and not having that information definitely seems like a usability regression.
The span meant for this information, with the class "summary," is empty. For instance, here's the code for the Menu Settings tab:
The span with class "summary" is also being hidden with CSS:
The CSS comes from the last definition in entity-meta.css (in core/themes/seven/css/components) and has the comment "Hide JS summaries. @todo Rethink summaries." That was originally written in the patch from #1838114-23: Change node form’s vertical tabs into details elements (subsequently worked on and committed in #1838114-81: Change node form’s vertical tabs into details elements). It's not clear to me reading that issue why the summaries weren't working, but presumably the fact that they're empty is why they're being hidden.
If someone else is more familiar with that history, maybe they could suggest what/how we should rethink those summaries, because it'd be great to get them working again.
Comment #3
yoroy CreditAttribution: yoroy commentedThanks for the investigations. Seems like a bug at this point as it would be a usability regression to not bring summaries back.
Comment #4
justinchev CreditAttribution: justinchev commentedI've been taking a look at this and trying to see if I can find a working example. I've gone as far back as D8 Alpha 2 and it's not in there.
Seems that these threads are relevant:
https://www.drupal.org/node/1838114#comment-6952220 - See patch on comment #6 as it seems to have some reference to it.
https://www.drupal.org/node/1751312 - Talks about re-writing the JS behaviours relevant to this
Anyway having delved into the current beta1 code I see that there is what looks like references to creating summary info within this file, but I couldn't decipher exactly how it was supposed to work (bit over my head),:
/core/modules/node/node.js
Anyway I've added a bit of jQuery (see patch) which provides this basic functionality and could potentially be used as a basis. It's rather messy at the moment with inline CSS, and no translation options etc, but I didn't want to spend too much time at this stage without getting some input on whether this is a viable/good way to approach this (I suspect this could be done in a better way).
PS I see that this is logged under Component 'Seven Theme' although all I found Seven Theme was doing is creating the Summary bit that appears at the top of the Tabs where the Published, last saved, author and revision info is showing)
Comment #5
LewisNymanComment #6
LewisNymanThis is not a Seven issue
Comment #7
nod_Not really novice, but let's see if someone is up for something a little challenging.
Comment #8
nod_Also patch in #4 needs quite a bit of work.
Comment #9
justinchev CreditAttribution: justinchev commentedYeah that patch needs a lot of work, but if you think achieving this with JS is fine I'll have a tinker.
Comment #10
sokru CreditAttribution: sokru commentedComment #11
sokru CreditAttribution: sokru commentedComment #12
sokru CreditAttribution: sokru commentedComment #13
nicolas@webstanz.be CreditAttribution: nicolas@webstanz.be commentedWe try to apply the patch (#4) with git apply but it display an error.
Comment #14
nicolas@webstanz.be CreditAttribution: nicolas@webstanz.be commentedPatch corrected with good indentations.
Also adds the Drupal T function. Ready to be improve.
Tks anansi
Mentored by Chipway
made @ DrupalConEur Barcelona
Comment #15
rootworkComment #18
bendev CreditAttribution: bendev at WebstanZ commentedtested patch in #14 . I can see the content between brackets but it is not updated.
Although I changed the url alias, it stays "No alias"
Comment #19
espurnesAfter some time looking at the functionality I realised that it is working as expected. The problem is that there is a css property hidding the summary tag. No need to apply any patch to /core/modules/node/node.js
The css property is in line 57 of /core/themes/seven/css/components/entity-meta.css
Proposed solution
Remove lines 57 to 59 of entity-meta.css file and style the summary tag using style guide documentation.
Remaining tasks
Define style for summary tag. There is no style information about this kind of element (https://groups.drupal.org/node/283223#Details_and_Accordion). Currently it's shown in uppercase and black.
Comment #20
espurnesThere two a discussions started 3 years ago related to this issue:
- https://www.drupal.org/node/1919956
- https://www.drupal.org/node/1838114#comment-6774686
No idea about the motivations to add display:none to summaries.
Comment #21
espurnesAdded Barcelona2015 tag to the issue.
Comment #22
bendev CreditAttribution: bendev at WebstanZ commentedI did the same check this morning in the plane (back from drupalcon barcelona)
I started from D7 and I I came to the same conclusion.
For those interested, the code is in the js of each module (corresponding to the tabs):
E.g : core/module/path/path.js
Drupal.behaviors.pathDetailsSummaries =
the same for pathauto etc...
By the way I tested your patch and it is ok.
I agree the style should likely be like on the first screenshot in the issue description...
todo @css
Comment #23
espurnesI checked the style of the summary element in D7:
So maybe we can apply the same style in D8 in the entity-meta.css
I think it's better to not force to show the summary value in uppercase.
Comment #24
bendev CreditAttribution: bendev at WebstanZ commented#23 tested.
this looks ok.
Thanks
Comment #25
bendev CreditAttribution: bendev at WebstanZ commentedComment #26
bendev CreditAttribution: bendev at WebstanZ commentedIt looks like it is working in firefox but neither in chrome nor in safari.
Could anyone else confirm?
Comment #27
espurnesI confirm the patch is working in firefox but not in chrome nor safari.
The next code is applied in firefox but not in the other browsers:
Anyone can check why the html is different?
Comment #28
bendev CreditAttribution: bendev at WebstanZ commentedthe code is in collapse.js locate in core/misc/ directory
Comment #29
sokru CreditAttribution: sokru at Druid commentedOnly webkit supports HTML5
<details>
element and I dont know why following code (in collapse.js) is there in first place, but that's causing summaries not to be visible in Chrome.Comment #30
rootwork#23 doesn't include the changes from #14. We need a patch that integrates that.
In terms of the font size, per #2045473: Improve visibility of Seven's smallest font elements the smallest we should make text is 1em. I agree on unsetting the all-caps text transform, but perhaps the parent item's text transform could be more specifically set so that we don't have one rule applying it and another rule removing it.
If you're curious about the source of this regression, see #2.
Comment #31
espurnesapplying #23 and #14 not solves the problem.
#14 adds additional markup and #23 just works in firefox... so it needs more work.
Comment #32
espurnesI commented code from /core/misc/collapse.js and the summary is visible in firefox (40.0.3), Chrome (45.0.2454). and Safari (6.2.8).
So we have to look deeper in this file and change the condition that prints the output.
Or maybe change the output to use different element than details or summary. Maybe a span or div?
Test with patch #23 applied.
Thanks sokru.
Comment #33
espurnesI ckecked again the functionality without the modernizr.details code and it works perfect in firefox, chrome and safari.
caniuse.com
Maybe the functionality show/hide is not working in some browsers but if the information is displayed this is not a problem from my point of view.
Other option is to use different tags like I said in #32, span or div tags can work.
Remaining tasks
- Test the functionality in other browsers
- Check if this patch affects other functionalities
Comment #34
espurnesComment #35
bendev CreditAttribution: bendev at WebstanZ commentedI tested #33 and it works as you described
we could replace the return in the check by something inspired from this in order to allow some specific handling
// @js: Use Modernizr to check the navigator capacities
if( !Modernizr.details ) jQuery(“html”).addClass(“no-details”);
source http://html5doctor.com/the-details-and-summary-elements/
Modernizr.details returns
fierefox => false
chrome => true
safari => true
Comment #36
espurnesworking on bendev solution.
details is used to show/hide extra information in a way like an accordion does. This is css3 native (so, no need to use javascript). But right now just the following browsers support the details element:
Chrome 12+
Opera 15+
Safari 6+
iOS Safari 6.1+
Android Browser 4+
Blackberry Browser 10+
Opera Mobile 30+
Chrome dor android 45+
UC Browser for Android 9.9+
So IE, Edge, Firefox, Opera Mini, Firefox for Android and IE Mobile don't support it.
But at least in Firefox the open/close functionality doesn't work but the content is displayed. So the bendev approach extracted from here is a good option. We set a new class to tell the browser doesn't support details.
While I was writting this message I realised the classes .details or .no-details have been already setted to the html element. I can't find the code that adds these classes but they are there.
So no need to add the bendev code. I think patch #33 should work.
Remaining tasks
- Test the functionality in other browsers
- Check if this patch affects other functionalities
Comment #37
espurnesComment #38
espurnesOk the class is added by Modernizr.details.
But after applying #33 (that deletes the use of Modernizr.details in /core/misc/collapse.js) the class is added anyway. I can't find another place where Modernizr.details is called.
Aggregation of css and js is disabled and I cleared the cache.
Remaining tasks
- Test the functionality in other browsers
- Check if this patch affects other functionalities
source:https://modernizr.com/docs#using-modernizr-with-css
Comment #39
bendev CreditAttribution: bendev at WebstanZ commentedIn my option this can go RTBC
Comment #40
bendev CreditAttribution: bendev at WebstanZ commentedI noticed that the summary was missing on the comment tab
I adapted the corresponding JS to fix this
I propose the new patch in attachment for review
Comment #41
jaspreetsingh31 CreditAttribution: jaspreetsingh31 at Axelerant commentedI have tested the patch in multiple browsers like Chrome ( version: 45.0.2454.101 ) , Firefox ( version:41.0.1) , Microsoft Edge and in terms of functionality it works fine. But In chrome browser, it shows twice arrows but seems fine on FireFox and Microsoft Edge browser.
For sake reference, Please find the attachments.
doublearrow_bug_chromebrowser.jpg & Firefox browser_ screenshot.jpg
Thanks
Jaspreet singh
Comment #42
bendev CreditAttribution: bendev at WebstanZ commentedOK I think we finally understand the point.
we need to use the no-details class added automatically to firefox-like browsers (see #36 and #37) in order to display the sumarry arrows so that they don't appear twice in the other browsers...
I will work on this soon
Comment #43
bendev CreditAttribution: bendev at WebstanZ commentedHere is the reroll of patch #40 for 8.1.X-dev
what remain to be done is #41
+ check the summaries from menu which are gone (perhaps another issue for this)
Comment #44
bendev CreditAttribution: bendev at WebstanZ commentedSorry wron patch in #43
here it is with the fix for the double arrows . this one needs review
Can anyone confirm me if we are supposed to work against 8.1.x-dev as I assumed ?
+to do create another issue for the summary of menu tab
Comment #45
nod_8.1.x is outdated, nothing is happening there, should do the work on 8.0.x since Patch doesn't apply and we can't test it :)
Comment #46
nod_Polished the code, it works both with chrome and firefox. We could even split up the code more but let's leave that for another time.
Fixed a few inacurate comments and remaining eslint issues.
Comment #47
bendev CreditAttribution: bendev at WebstanZ commentedtestd in chrome firefox and safari
Comment #48
jaspreetsingh31 CreditAttribution: jaspreetsingh31 at Axelerant commentedI have tested this double arrow issue on multiple browsers like chrome, Firefox, Microsoft edge and it works fine.
Status: Done
For sake reference, Please find the attachments.
Thanks
Comment #50
nod_Most of that patch is in #2493957: Add back errors support to native HTML5 details tag.
Comment #51
emma.mariaRemoving the Novice tag as this issue needs some new direction for what the new patch needs to contain.
Comment #52
espurnesRerroled patch #46
Comment #53
droplet CreditAttribution: droplet commentedCompare to #2493957: Add back errors support to native HTML5 details tag:
- Both patched little more than main problem of issue thread.
- Both are good patches but slightly different on few code style. #2493957 is more close to other core scripts. But I like this patch move following code into `CollapsibleDetails`
From my quick review of this patch, only missing following code from another issue thread:
To standardize the color.
Comment #54
espurnesI added suggestion from droplet #53 to collapse-processed.css
Comment #55
droplet CreditAttribution: droplet commentedComment #59
joelpittetAdding Novice for manual testing, needs screenshots.
Comment #62
joelpittetThis will need a re-roll and manual testing. Maybe manual test first to make sure this is still an issue to be solved before rerolling.
Comment #63
mbroere CreditAttribution: mbroere at LimoenGroen for gemeente Leidschendam-Voorburg commentedComment #64
darren.fisher CreditAttribution: darren.fisher at Pivale commentedI'm going to attempt to re-roll patch 54 now for 8.4.
Comment #65
kwoxer CreditAttribution: kwoxer commentedOn Drupal 8.4.0-dev there is no such information. I have no patch running.
See that shot to make sure I was on the right page or view:
So which patch should I test?
Comment #66
mbroere CreditAttribution: mbroere at LimoenGroen for gemeente Leidschendam-Voorburg commentedTested manually and re-rolled the patch. Works local.
Comment #67
mbroere CreditAttribution: mbroere at LimoenGroen for gemeente Leidschendam-Voorburg commentedComment #68
darren.fisher CreditAttribution: darren.fisher at Pivale commentedI'll test the patch in #66. Glad you've managed to re-roll.
Comment #69
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #70
joumHi everyone! I'm at Drupalcon Vienna2017 and I'm a bit confused by the status of this issue.
I just applied the 1936708-66.patch against 8.5.x-dev and it seems to be working as intended. (see screenshot)
What else needs to be done? (First time sprinter, instructions unclear... :)
EDIT:
Validated correct function in MacOSX 10.12.6 (16G29) using
Chrome 60.0.3112.113;
Safari 11.0 (12604.1.38.1.7);
Firefox 52.0.2 (64-bit)
Comment #71
joumComment #72
darren.fisher CreditAttribution: darren.fisher at Pivale commentedConfirming that patch in #66 is working for me too.
Comment #73
tonifisler CreditAttribution: tonifisler at Antistatique commentedConfirming that it works for me too. 👍
Comment #74
mbroere CreditAttribution: mbroere at LimoenGroen commentedComment #75
larowlanWe have JavaScript testing in core now, we need a test here so we don't lose it again.
Thanks everyone
Comment #79
vacho CreditAttribution: vacho at Skilld commentedRebasing
Comment #80
aleevasComment #82
tvb CreditAttribution: tvb commentedPatch applies cleanly to 8.9.x-dev.
Comment #83
tvb CreditAttribution: tvb commentedAfter applying the patch from #79 and checking out a node edit form:
1) the span tags with class summary are not empty (good),
2) but the span contents are not displayed in the UI (not good).
(Seven theme, on Firefox and on Chrome)
Commenting out display: none; produces a result similar to the result in #70, but this seems to conflict with the issue mentioned in the preceding @todo;
Not sure what the status should be, Removing the 'Novice' tag.
Comment #85
nod_doesn't apply cleanly on 9.1.x
Like tvb said, Need to remove the CSS that hides the summay, otherwise this patch doesn't change anything visually.
Comment #86
nod_Comment #87
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedComment #88
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch for 9.1.x and removed the CSS that hides the summary as mentioned in #85, please review.
Comment #90
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedTest Fixed, Please review.
Comment #91
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #92
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedVerified and tested by applying the patch #90.It was applied successfully. Can be moved to RTBC.
Had one observation for the Claro theme the text didn't display.
Steps to test -
1. Go to the admin site.
2. Go to admin/appearance.
3. Select theme Bartik/Claro/Seven/Default
4. Edit any of the created content type.
5. Verify.
Seven_theme_Chrome
Seven_theme_Safari
Bartik_theme_Safari
Bartik_theme_Chrome
Claro_theme_Safari
Claro_theme_Chrome
Comment #93
nod_When you create a JavaScript patch please make sure to always run
yarn run lint:core-js-passing
before sending the patch, for the whole workflow have a look here: https://www.drupal.org/node/2986680Lately the JS patches have had issues with not following core standards so please make sure to use the tools we have in core to reduce the workload of reviewers, Thank you.
Claro overrides some things about collapse.js, needs to be fixed.
Comment #94
bnjmnmI noticed most of the logic was modifying collapse.js. That is a polyfill for browsers that don't support
<details>
. Browsers that didn't need the polyfill were getting some of it anyway, and I think it may prove difficult to maintain. I opted to move the functionality to a dedicated JS file.The CSS changes are pretty straightforward. Claro needed some additional JavaScript changes to remove its own workarounds for this issue, otherwise summaries would be duplicated in some instances.
Comment #95
bnjmnmComment #96
bnjmnmComment #97
bnjmnmAdded test. The test addition is the only change, but there is a dedicated interdiff as a bit of formatting was changed.
Comment #99
bnjmnm1936708-97-TEST-ONLY failing was expected, so switching back to NR.
Comment #100
bnjmnmRe-adding with the Test-only patch first so testbot doesn't kick back to Needs review.
Comment #102
nod_you can name it
--FAIL.patch
instead of TEST-ONLY.patch. It'll do what you want :)Looking at it, it definitely should be in the collapse file. Because now there are a lot of duplications. There is a theme function in the details.js file, but the same thing is hardcoded in the collapse.js file. What is done in details.js is a light version of collapse.js. some of what's in claro details.js could be in core, I don't see why we can't have summary tags trigger when hitting space everywhere.
Comment #103
bnjmnmAh, that's right, collapse,js is a polyfill but it doesn't replace details.js, it runs in addition to it.
My concern is that this file has been long understood as strictly being a polyfill for browsers that don't support
<details>
. There has been interest in replacing this with a different polyfill #1839158: Replace collapse.js with a proper polyfill for <details>, and this would be made far more difficult by splicing in functionality that applies to all browsers.If there is still a preference to include the fix in collapse.js, there would be a need to edit several comments in the file that reference itself as a polyfill, which it would no longer be, starting with
@file
,My preference would be to address the duplication by removing the summary logic in collapse.js, and make sure that the new summary logic is compatible with the collapse.js polyfill
I'm I correct in interpreting that this is a reference to the shim that's already there, not the changes i the patch? That does seem like something better applied to core. I believe it would be out of scope here, but tagging with needs followup as a reminder to create that issue.
Comment #104
nod_Right, like you said, moving the code for
setupSummary
from collapse.js to details.js is good.And yes about the claro code, it's about what already exists you're correct.
Comment #105
bnjmnmCreated followup #3160367: Move Claro's polyfills in details.es6.js to core for the claro functionality that should be in core.
Removed the now-redundant code from the collapse polyfill.
Comment #106
bnjmnmThe patches that should have been with #105
Comment #107
lauriiiChecked that there aren't any instances where it would be expected that
this.setupSummary
gets called http://grep.xnddx.ru/search?text=setupSummary&filename=I'm pondering if we should make this more specific to the functionality we're providing. The current names sound very generic 🤔
What are the benefits of splitting this into two functions?
detailsSummarySummary
sounds a bit strange. Would be great if we come up with a name that doesn't require using summary twice.Comment #108
bnjmnmThis addresses #107
An explanation for how I addressed item 3:
It was two separate functions in collapse.js, but looks like the separation was no longer needed after #1685140: Refactor collapse.js. They were separate to address cover Drupal's former use of fieldset+legend to achieve details+summary like behavior. I did some refactoring of both details and the polyfill to make this clearer, and checked http://grep.xnddx.ru/ to ensure the refactoring wouldn't cause problems for contrib.
Comment #109
lauriiiBased on the discussion on Slack with @bnjmnm we should still consider if we could improve the naming to be more descriptive on what is actually being done.
DetailsEnhanced
sounds like the details element is enhanced which is not 100% accurate because what we're actually doing, is just generating summary for the details based on form input.Comment #110
bnjmnmDid some renaming (and refactoring due to it)
Comment #111
bnjmnmForgot the patch in 110 😛
Comment #112
nod_Synopsis does not work for me: It's a complicated word, the meaning is not obvious, it's hard to say (for me at least). For exemple I honestly do not know how to pronounce:
synopsized
.I would suggest brief, then we have names like: DetailsBriefSummary, Drupal.theme.detailsBriefSummary, Drupal.theme.detailsBriefSummaryText, etc. It makes sense when used both as a noun or an adjective. It's not an usual word used in webdev though.
A better one might be outline, benefit is that it's a well known term in HTML land, accessibility, in CSS (with a different meaning but still the word is already known). And it doesn't rely on a pun to be useful. DetailsSummaryOutline it's more descriptive, it's an outline inside a details inside a summary.
In any case, not synopsis.
Comment #113
bnjmnmThose are both much better options that synopsis, thanks!
I opted for outline. Both brief and outline have multiple meanings that could potentially be confusing, but when seen in context I think the confusion for "outline" is mitigated more quickly.
Comment #114
nod_We can probably get away with DetailsOutline, the fact that it's in the summary element is kind of an implementation detail. In any case, much better already, thanks!
Comment #115
lauriiiSince this summary is at least at the moment only being used in forms, how about we call it
DetailsFormSummary
? Or if we think we should be open to outside of form implementations, we could useDetailsGeneratedSummary
. The reason I like summary is that the API is still using the word summary in other places, for exampledrupalGetSummary
andsummaryCallback
.Comment #116
bnjmnmAt the Claro weekly check-in we discussed the naming and agreed on
DetailsSummmarizedContent
. Summary is the most accurate term, and using "Summarized" eliminated the confusion with the<summary>
element. The patch that renames it should also see if any comments need to be changed to reflect the renaming.Comment #117
bnjmnmRenamed per #116
Comment #118
paulocsThat was fast! I was pretending to assign the issue to me when I finished to read comment #116 but I was to slow hahaha
So if patch #117 passes on tests, I will test it. Thanks @bnjmnm!
Cheers, Paulo.
Comment #119
paulocsPatch #117 looks good to me!
I tested with Seven and Claro Theme. (I attached images to prove it)
Another point is that the patch should use DetailsSummmarizedContent term and I can confirm that the patch do it right.
All comments mention the right term too.
Set to RTBC.
Cheers, Paulo.
Comment #120
lauriiiWe are not adding the instances to the list at the moment so this is always empty.
Should we prepend a whitespace to the beginning of the text like the default instance of this theme function does?
Comment #121
bnjmnm#120.1 Removed that extend.
#120.2 Whitespace shouldn't be appended in Claro as its design has the extended summary is enclosed in a block so it appears on the next line.
Comment #122
nod_The instances property was used here for 2 reasons: 1. allow scripts to access the created objects in case they need an override of some kind. 2. not have the eslint warning about using new for sideeffects.
We should keep the created instances so they're accessible from the outside so I'd do:
Comment #123
bnjmnmThanks @nod_, that explanation helps make sense of why it's done and better way to do it.
Comment #124
nod_That's good with me :) thanks
Comment #125
lauriiiThis looks pretty good for me too. Here's few minor changes we still have to do. Feel free to move this back to RTBC once they have been addressed.
These are referring to a non-existing variable. These should be renamed to
--color-davysgray
.Comment #126
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedMade the changes suggested in #125. Please review
Comment #127
bnjmnmThanks @ayushmishra206
In #126
1. Updates the source accordion.pcss.css, but the complied version isn't present and the unchanged CSS would be loaded
2. Changes the compiled details.css, when the changes could be made to details.pcss.css then complied to details.css.
More on how Claro uses PostCSS to build CSS can be found here https://www.drupal.org/node/3084859
Updated patches attached and returning to RTBC as #125 said that would be fine.
Comment #129
lauriiiCommitted 6e6ad99 and pushed to 9.1.x. Thanks!