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

Move CDN CLF libraries to hook_library_info_build() #79

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joelpittet
Copy link
Collaborator

This is a clean-up thing because the libraries file was getting long and there is lots of duplication happening in there.

I have not tested that this works... yet. So this is more of a drive by Proof of Concept. Needs a quick test and review before merging

@joelpittet joelpittet requested a review from occupant September 24, 2024 06:25
@joelpittet
Copy link
Collaborator Author

Ok I tested it and it seems to work. Now the question is if this is valuable @occupant? My motivation was to shrink the libraries file down.

@occupant
Copy link
Member

@joelpittet Nice. I like this, but I wonder if "hiding" the libraries in a theme hook is easier or harder to manage? The "advantage" to having them in the libraries file is they are all in one place and use the same method of addition. I'm sure the method of adding the library is the same under the hood regardless of which approach is used (hook vs libraries file), so it's more a question of having one approach / place to manage vs two. That said, I think it's entirely appropriate that they get moved, if for no other reason than these libraries are not intended to be easily editable.

It drives me a little bonkers that these CSS files exist as separate items - this is a perfect example of where css variables would be perfect. One file and a small selection of variables could easily handle this much more efficiently. Oh well, one can dream.

So, my feeling is it's worth changing (and you obviously feel the same :) ) so let's make the update. It looks like hook_page_attachments_alter will just work the same.

Copy link
Member

@occupant occupant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good to go!

@joelpittet
Copy link
Collaborator Author

I am hesitant because I agree with you that it's moving out of a common place people look to a more "advanced" method of adding libraries. The only reason was there was so much duplication in that yaml file it was annoying to scroll past, but not sure if that is reason enough to move it out. Also because it's in a loop now if you want to make config changes you can do it to all them in one big move... so on the fence with you :)

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

Successfully merging this pull request may close these issues.

2 participants