-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
Add Append StyleSheet Pack Tag Helper #144
Changes from 8 commits
6873d42
8c1d958
eed566f
4b8c45d
1f72772
32e4d3f
1d2157e
dd6d56a
118686b
7e8acbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -158,7 +158,27 @@ def preload_pack_asset(name, **options) | |
def stylesheet_pack_tag(*names, **options) | ||
return "" if Webpacker.inlining_css? | ||
|
||
stylesheet_link_tag(*sources_from_manifest_entrypoints(names, type: :stylesheet), **options) | ||
if @stylesheet_pack_tag_loaded | ||
puts "To prevent duplicated stylesheet on the page, you should call stylesheet_pack_tag only once on the page. " \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, won't you get the warning here even for the perfectly fine case of
Am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed over DM, removed the warning for now 😄 |
||
"Please refer to https://github.com/shakacode/shakapacker/blob/master/README.md#usage for the usage guide" | ||
end | ||
|
||
requested_packs = sources_from_manifest_entrypoints(names, type: :stylesheet) | ||
appended_packs = available_sources_from_manifest_entrypoints(@stylesheet_pack_tag_queue || [], type: :stylesheet) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need any deduplication? Can names passed and what's in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good catch, Handled it by using |
||
|
||
@stylesheet_pack_tag_loaded = true | ||
|
||
stylesheet_link_tag(*(requested_packs | appended_packs), **options) | ||
end | ||
|
||
def append_stylesheet_pack_tag(*names) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think we need more than just names here. If you look at original issue around multiple pack tags, one of the requests was supporting different "media" attributes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can Queue Up the files based on media attributes as a separate Feature Pull Request as, unlike deferred, it's not a boolean value and can have Example: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tomdracz I'm not convinced how many people would need both the flexibility of the Let's wait for feedback. If necessary, we release an update where the
@tomdracz do you agree that this would be a backwards compatible update if needed? @pulkitkkr are we ready to merge and release this one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @justin808 If I remember correctly we've removed the raise in the first place to support different The update if needed should be backwards-compatible, yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tomdracz should we document the ordering for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pulkitkkr we need to document what happens when this happens: <%= stylesheet_pack_tag "application" %>
<%= stylesheet_pack_tag "print", media: "print" %> See #39 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have :
|
||
if @stylesheet_pack_tag_loaded | ||
raise "You can only call append_stylesheet_pack_tag before stylesheet_pack_tag helper. " \ | ||
"Please refer to https://github.com/shakacode/shakapacker/blob/master/README.md#usage for the usage guide" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please refer to the documentation for stylesheet_pack_tag for more information. |
||
end | ||
|
||
@stylesheet_pack_tag_queue ||= [] | ||
@stylesheet_pack_tag_queue.concat names | ||
end | ||
|
||
def append_javascript_pack_tag(*names, defer: true) | ||
|
@@ -184,6 +204,10 @@ def sources_from_manifest_entrypoints(names, type:) | |
names.map { |name| current_webpacker_instance.manifest.lookup_pack_with_chunks!(name.to_s, type: type) }.flatten.uniq | ||
end | ||
|
||
def available_sources_from_manifest_entrypoints(names, type:) | ||
names.map { |name| current_webpacker_instance.manifest.lookup_pack_with_chunks(name.to_s, type: type) }.flatten.compact.uniq | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pulkitkkr What's the rationale for adding this? Why would we use non-raising method that does not indicate pack missing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tomdracz We are introducing a new feature to Eg: Developer used This will automatically load, While testing this on one of the projects, we discovered there were few component packs with |
||
end | ||
|
||
def resolve_path_to_image(name, **options) | ||
path = name.starts_with?("static/") ? name : "static/#{name}" | ||
path_to_asset(current_webpacker_instance.manifest.lookup!(path), options) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -204,5 +204,50 @@ def test_stylesheet_pack_tag_multiple_invocations_are_allowed | |
assert_equal \ | ||
hello_stimulus_stylesheet_chunks.map { |chunk| stylesheet_link_tag(chunk) }.join("\n"), | ||
stimulus_style | ||
|
||
assert_nothing_raised do | ||
stylesheet_pack_tag(:application) | ||
stylesheet_pack_tag(:hello_stimulus) | ||
end | ||
end | ||
|
||
def test_stylesheet_pack_with_append | ||
append_stylesheet_pack_tag(:hello_stimulus) | ||
|
||
assert_equal \ | ||
(application_stylesheet_chunks + hello_stimulus_stylesheet_chunks).uniq.map { |chunk| stylesheet_link_tag(chunk) }.join("\n"), | ||
stylesheet_pack_tag(:application) | ||
end | ||
|
||
def test_stylesheet_pack_with_duplicate_append | ||
append_stylesheet_pack_tag(:hello_stimulus) | ||
append_stylesheet_pack_tag(:application) | ||
|
||
assert_equal \ | ||
(application_stylesheet_chunks + hello_stimulus_stylesheet_chunks).uniq.map { |chunk| stylesheet_link_tag(chunk) }.join("\n"), | ||
stylesheet_pack_tag(:application) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests don't address:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (and possibly both with the same and different stylesheet names). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alexeyr We already have a one for different names There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant with different names and different media attributes, which that test doesn't do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated The Test to handle this scenario :) |
||
|
||
def test_multiple_stylesheet_pack_with_different_media_attr | ||
app_style = stylesheet_pack_tag(:application) | ||
app_style_with_media = stylesheet_pack_tag(:application, media: "print") | ||
hello_stimulus_style_with_media = stylesheet_pack_tag(:hello_stimulus, media: "all") | ||
|
||
assert_equal \ | ||
application_stylesheet_chunks.map { |chunk| stylesheet_link_tag(chunk) }.join("\n"), | ||
app_style | ||
|
||
assert_equal \ | ||
application_stylesheet_chunks.map { |chunk| stylesheet_link_tag(chunk, media: "print") }.join("\n"), | ||
app_style_with_media | ||
|
||
assert_equal \ | ||
hello_stimulus_stylesheet_chunks.map { |chunk| stylesheet_link_tag(chunk, media: "all") }.join("\n"), | ||
hello_stimulus_style_with_media | ||
|
||
assert_nothing_raised do | ||
stylesheet_pack_tag(:application) | ||
stylesheet_pack_tag(:application, media: "print") | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect.