Skip to content
New issue

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

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

Already on GitHub? # to your account

Add asset processing for script tags #332

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

ctron
Copy link
Collaborator

@ctron ctron commented Mar 17, 2022

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).
  • Updated site content with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done. If you don't, then Dodd will 🤓.

This change adds support for processing <script> tags with the asset pipeline in a way similar to <link rel="css"> assets. This fixes the request originally posted by @hobofan in #3 (comment)

@ctron ctron changed the title Add assset processing for script tags Add asset processing for script tags Mar 17, 2022
@ctron ctron force-pushed the feature/js_asset_1 branch from b79c8de to b5579de Compare March 17, 2022 09:08
@ctron ctron marked this pull request as ready for review March 17, 2022 09:11
Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

This is looking pretty awesome, thanks for this work! I would ask that you integrate this feature into one of the current examples as a way to be able to test this feature.

As unit/integration testing is not nearly where it needs to be in this system, this is one of the primary ways to ensure features continue to work as needed (as well as first-line verification).

@ctron ctron force-pushed the feature/js_asset_1 branch from b5579de to 92628c5 Compare June 30, 2022 17:31
@ctron
Copy link
Collaborator Author

ctron commented Jun 30, 2022

I rebased the PR. And added its use to the vanilla example. I hope this is what you had in mind.

Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Really great work here! Just two small items, then I'll merge. Thanks boss.

@ctron ctron force-pushed the feature/js_asset_1 branch from 84fac4b to 1e0b940 Compare July 4, 2022 06:50
@ctron
Copy link
Collaborator Author

ctron commented Jul 4, 2022

Glad you like it! I amended the PR with your recommended changes. I agree, the name is more appropriate now. I just didn't want to change that many file upfront :)

ctron and others added 2 commits July 4, 2022 08:53
Co-authored-by: Anthony Dodd <dodd.anthonyjosiah@gmail.com>
@ctron ctron force-pushed the feature/js_asset_1 branch from 1e0b940 to 004344e Compare July 4, 2022 06:53
Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for all of the great work on this feature.

@thedodd thedodd merged commit a4289dd into trunk-rs:master Jul 6, 2022
@ctron ctron deleted the feature/js_asset_1 branch July 6, 2022 06:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants