Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue from "feat: add expand/collapse icon to accordion" #626

Closed
ekes opened this issue Sep 26, 2024 · 4 comments
Closed

Issue from "feat: add expand/collapse icon to accordion" #626

ekes opened this issue Sep 26, 2024 · 4 comments
Assignees

Comments

@ekes
Copy link
Member

ekes commented Sep 26, 2024

#603

One thing I spotted on the "front end" is that #603 adds icons as expected, however this adds them to all sites even if they have overwritten the Twig template to introduce their own icons:
image
Whilst the addition of the icons is good I don't think it is wise that these are added in using JavaScript (we also add the button using this method too) - I have a PR (localgovdrupal/localgov_paragraphs#200) that @ctorgalson has reviewed and approved which moves the creation of the button and the addition of the icons into the Twig template which I think should be released along with localgov_base v1.7.0

Originally posted by @AWearring in #622 (comment)

@ekes ekes assigned ekes, markconroy and ctorgalson and unassigned ekes Sep 26, 2024
@ekes
Copy link
Member Author

ekes commented Sep 26, 2024

@markconroy @ctorgalson Do we want to resolve this before pushing #622 out the door?

@ctorgalson
Copy link
Contributor

ctorgalson commented Sep 26, 2024

So my TL;DR is I don't understand what difference it makes in what order they're deployed.

Details

I'm not very well-informed about v1.7.0, but localgovdrupal/localgov_paragraphs#200 still looks fine to me as long as everyone's aware of those minor potential side effects noted in the PR.

Regarding the base issue:

One thing I spotted on the "front end" is that #603 adds icons as expected, however this adds them to all sites even if they have overwritten the Twig template to introduce their own icons:

The paragraphs change deom localgovdrupal/localgov_paragraphs#200 won't prevent that.

Unless I'm overlooking something fairly large, there are only three ways that that could be prevented:

  1. don't render the icons in localgov_accordion Twig or javascript,
  2. provide an identical or more specific :after selector in a child theme to override localgov_base's new css
  3. ensure, in a child theme, that the <button> element in accordion panes doesn't match the selector in localgov_base's new css

The changes in localgovdrupal/localgov_paragraphs#200 don't do any of these things: they duplicate the previous markup via Twig instead of js, and address the side-effects that creates in the accordion javascript.

@ekes ekes mentioned this issue Sep 26, 2024
@finnlewis
Copy link
Member

@AWearring suggests that maybe this is OK, with release notes to let people know that we're adding icons, so if they already have icons, they might need to override our template.

@markconroy also keen to cover this in release notes.

@ekes
Copy link
Member Author

ekes commented Oct 22, 2024

1.7.0 is out

@ekes ekes closed this as completed Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants