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

Adds option extname to stylesheet_link_tag to exclude automatically appended .css extension #41927

Merged

Conversation

abhaynikam
Copy link
Contributor

@abhaynikam abhaynikam commented Apr 12, 2021

Fixes: #41918

This PR adds the option extname to stylesheet_link_tag.
Previously, stylesheet_link_tag automatically added .css.

The change is similar to javascript_include_tag with extname option.

@rails-bot rails-bot bot added the actionview label Apr 12, 2021
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

Testing this with <%= stylesheet_link_tag 'something.scss', extname: false, skip_pipeline: true %> seems to 404 when the browser fetches the scss. Using the pipeline raises a sprockets error Sprockets::Rails::Helper::AssetNotPrecompiled. I'm trying to clarify the use-case for this on the issue, I might be missing something.

Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

Please also add a changelog.

Regardless of the vite use-case, you can compile less in-browser officially with a JS library, so this makes sense to me. You would need to skip the pipeline though and service it from public though. Probably not a good idea for production, but I can see a reason to do it in development mode.

@@ -138,6 +155,9 @@ def javascript_include_tag(*sources)
# stylesheet_link_tag "http://www.example.com/style.css"
# # => <link href="http://www.example.com/style.css" rel="stylesheet" />
#
# stylesheet_link_tag "style.less", extname: false
Copy link
Member

@gmcgibbon gmcgibbon Apr 28, 2021

Choose a reason for hiding this comment

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

I think

Suggested change
# stylesheet_link_tag "style.less", extname: false
# stylesheet_link_tag "style.less", extname: false, skip_pipeline: true, rel: "stylesheet/less"

would be more correct. See Less' example: https://lesscss.org/, skip_pipeline: true may not be needed.

… appended `.css` extension.

Fixes: rails#41918

This PR adds the option `extname` to `stylesheet_link_tag`.
Previously, `stylesheet_link_tag` automatically added `.css`.
@abhaynikam abhaynikam force-pushed the 41918-fix-stylesheet-link-tag-without-extname branch from 5528e83 to 2005afb Compare April 29, 2021 05:51
@abhaynikam
Copy link
Contributor Author

@gmcgibbon Thanks for taking look at the PR 😊 . Addressed all the suggestion and tested it locally.

@gmcgibbon gmcgibbon merged commit e0d2459 into rails:main Apr 29, 2021
@abhaynikam abhaynikam deleted the 41918-fix-stylesheet-link-tag-without-extname branch April 29, 2021 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stylesheet_link_tag should can use extname param
2 participants