-
-
Notifications
You must be signed in to change notification settings - Fork 933
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
Stimulus controller structure and stimulus-rails gem #4417
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4417 +/- ##
=======================================
Coverage 96.99% 96.99%
=======================================
Files 349 349
Lines 7695 7695
=======================================
Hits 7464 7464
Misses 231 231 ☔ View full report in Codecov by Sentry. |
config/importmap.rb
Outdated
# stimulus-loading.js is a compiled asset only available from stimulus-rails gem | ||
pin "@hotwired/stimulus-loading", to: "stimulus-loading.js" | ||
pin_all_from "app/javascript/controllers", under: "controllers" | ||
pin "clipboard", preload: false # @2.0.11 |
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.
Why the change to preload false?
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.
These are only used on specific pages. Seemed aggressive to preload them.
This change snuck in with my overall changes though and wasn't really intended for this commit.
Do you think I should revert this?
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.
It's fine so long as it's intentional and you've tested that it doesn't break the clipboard functionality
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.
I'll revert for now. I want to cleanup the loading but I'll do it in a separate PR.
62d3ce4
to
ee45981
Compare
ee45981
to
af8f7a4
Compare
@@ -1,13 +1,18 @@ | |||
# Pin npm packages by running ./bin/importmap | |||
|
|||
pin "application", preload: true |
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.
Note: preload: true
is the default.
This loads, but otherwise ignores stimulus. I'll use this as a base to convert each of the javascript features to stimulus.