removed INJECTION const that when injected affected certain styles on…#959
Conversation
… page. Extracted reload script to its own js folder to resolve css issues. Added in condition to load script if in development in application layout and routes.
|
@samholst This is great! thanks for working on this. 💯 Is it ready to be reviewed? |
|
@drujensen Thanks! In just a few it should be ready for review :) |
|
@faustinoaq was working on something similar here: #865 @drujensen @faustinoaq Perhaps we should include the JS file optionally based upon a variable set or added by the |
elorest
left a comment
elorest
left a comment
There was a problem hiding this comment.
A couple comments but I think this is great progress. I've added a PR as well just to get the feature back but I'll hold off on merging it as I think this could end up better.
Another option would be to add a string to context which would be empty by default but could be used to inject code into the write parts of the html body.
The problem with injecting it in the beginning or response, is that it's outside of the html body, which works for some reason but has weird implications on style.
| <script src="https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.14.0/umd/popper.min.js"></script> | ||
| <script src="https://stackpath.bootstrapcdn.com/bootstrap/4.1.0/js/bootstrap.min.js"></script> | ||
| <script src="/dist/main.bundle.js"></script> | ||
| <%="<"%>%- if Amber.env.development? -%><script src="/js/amber_reload.js"></script><%="<"%>%- end -%> |
There was a problem hiding this comment.
This is where I was saying we could check a variable set by the pipeline.
#in reload_pipe
context.params["_auto_reload_"] = "true"
call_nextThen in the layout check if that param exists instead of checking the environment.
What do you think @drujensen
| script src="https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.14.0/umd/popper.min.js" | ||
| script src="https://stackpath.bootstrapcdn.com/bootstrap/4.1.0/js/bootstrap.min.js" | ||
| script src="/dist/main.bundle.js" | ||
| - if Amber.env.development? |
There was a problem hiding this comment.
Talked to @elorest about this. Let's create a new setting for this that we can turn on and off instead of based on which environment you are in. Something like if Amber.settings.auto_reload?
| @@ -0,0 +1,23 @@ | |||
| require "../../support/client_reload" | |||
There was a problem hiding this comment.
This shouldn't be a pipeline since it doesn't actually modify the request/response in any way. Let's find a way to initialize the socket in the CLI when using amber watch. We should look for the same Amber.settings.auto_reload? flag and if it's true then we instantiate the socket.
…ng in Amber Settings and auto_reload property?
|
@elorest @drujensen Ready for review :) |
drujensen
left a comment
drujensen
left a comment
There was a problem hiding this comment.
This is so much better using a setting. Thanks!
| @@ -1,3 +1,4 @@ | |||
| require "../config/*" | |||
|
|
|||
| Amber::Support::ClientReload.new if Amber.settings.auto_reload? | |||
There was a problem hiding this comment.
I wonder if we can use an initializer for this instead of in the main program? wdyt?
There was a problem hiding this comment.
I'm for that. Let me make that change real fast.
| <% when "sqlite" -%> | ||
| database_url: sqlite3:./db/<%= database_name_base %>_development.db | ||
| <% end -%> | ||
| auto_reload: true |
| @@ -0,0 +1 @@ | |||
| Amber::Support::ClientReload.new if Amber.settings.auto_reload? | |||
There was a problem hiding this comment.
@drujensen I feel like initializers are for initializing shards or outside libraries rather than something built into amber. This is how Rails does it.
The fact that this is built into our JS, amber watch, and env settings means that this is clearly not something outside of amber.
Am I wrong here? Personally I though it made more sense in the main project file.
There was a problem hiding this comment.
I disagree. This is not really core to the framework and is an add-on. It should be easily removed. To your point, maybe the setting should be moved to the custom settings?
|
@elorest can we merge this? |
|
I talked to @elorest and we will go with an initializer for now in anticipation for making live_reload an add-on. |
4 participants
… page. Extracted reload script to its own js folder to resolve css issues. Added in condition to load script if in development in application layout and routes.
Description of the Change
Amber reload was previously removed due to unknown issues and at least for the projects I worked on in Crystal, it would inadvertently skew certain css stylings when added on the the
context.response. By extracting out the JavaScript file into in the public folder, it fixed the issues I was having with the previous code. Due to the extraction, a static socket route is need in order for the JavaScript to know what socket to connect to.Alternate Designs
Could possibly get a shard that is included by default in amber instead of a pipeline.
Benefits
Codebase and css styling functioned as wanted and were not effected by extra padding and positioning from the injected constant that was previously used.
Possible Drawbacks
May not qualify as a pipeline with the new changes.