Refs #23200 - Moving inline code to external scss files #421
Conversation
|
Issues: #23200 |
package.json
Outdated
| "uglifyjs-webpack-plugin": "^1.2.4" | ||
| }, | ||
| "dependencies": { | ||
| "babel-polyfill": "^6.26.0", |
There was a problem hiding this comment.
I would suggest not to load these unless you are adding code that uses it.
There was a problem hiding this comment.
I wish to start moving stuff slowly to react, that's why I'ved added them, at the moment I want to start with supporting of scss loading using .js file.
ik5
changed the title
|
Okay it looks great (from my side), but one thing is that I'm not sure here, does this package requires the tranformation and polyfills of babel, or will it be implemented by the main foreman asset manager. |
|
Thanks, honestly this patch is terrifying for me. I don't understand much of JavaScript and I foresee packaging and production issues with that. Are you willing to do all the packaging changes and production testing? But first let's get discovery tests fixed, then merge most of the other PRs and then let's talk more about this. Also this change should pull all the javascript which we have, which is exactly one inline block. I am not really sold in this. Why you use Refs instead Fixes? I see a separate issue for this. Should be Fixed imo. |
|
@lzap the changes are compatible with the way foreman is going. As I understand only new things or dependencies that obsolete are moving from jQuery to React, while I was starting to add features I have already found such cases. My task here is first to create such migration, and then the second task is to separate the inline Javascript and the inline CSS code into an external pipeline rather then Rails asset pipeline the way Foreman is currently working (aka stage 2). Note that at this point, the every Javascript and CSS changes are inline rather then using external files, so it will create a much cleaner code that easier to maintain in the long run, and create a bit more DRY code in a whole. The actual dependencies that I'm using, are also installed by foreman, and at this time I'm trying to figure out if I need to add that dependencies also on my own or just use them, and load them at the code directly. I am willing to get all the responsibility for this changes (otherwise I would not have taken this task on myself :) ) |
|
@sharvit can you help, and review this PR to see that for "bootstrapping" at least the css for now, it is good? Thank you |
|
@ik5 It looks fine but I wouldn't merge it before I see it is actually doing the job. Need to load the relevant compiled js and css into your views/layouts and see if it is actually work. |
|
I'ved tested it with SCSS, and it was compiled into the bundle, at this
time, after discussions, the javascript will not be changed, unless needed
to.
…On Sun, Apr 29, 2018 at 3:43 PM, Avi Sharvit ***@***.***> wrote:
@ik5 <https://github.com/ik5> It looks fine but I wouldn't merge it
before I see it is actually doing the job.
Need to load the relevant compiled js and css into your views/layouts and
see if it is actually work.
see:
https://github.com/theforeman/foreman/blob/develop/app/
views/layouts/base.html.erb#L13
https://github.com/theforeman/foreman/blob/develop/app/
views/layouts/base.html.erb#L18-L19
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#421 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEdg8YCAthMOyXba0ovBe3nxefedMmSks5ttbWKgaJpZM4TPoaI>
.
|
|
@ik5 but you still didnt include it in your layout? nor has any content in it, should you move one of the inline css (or all of them) as part of this pr? |
|
I want to add them (all of the inline css) in the next stage, this stage
was intended for the setup of the infrastructure.
…On Mon, Apr 30, 2018 at 9:25 AM, Ohad Levy ***@***.***> wrote:
@ik5 <https://github.com/ik5> but you still didnt include it in your
layout? nor has any content in it, should you move one of the inline css
(or all of them) as part of this pr?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#421 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEdg2l7X72knXy9VtQGPOwvLE5T728fks5ttq5lgaJpZM4TPoaI>
.
|
|
@ik5 I personally afraid to merge infrastructure and infrastructure-consumer separately. In my experience, many times, there are changes needed to be at the infrastructure. I think a part of me can never trust a code that never ran. |
ik5
changed the title
|
@ik5 I can't see any of your changes yet? please just let us know when a re-review is needed. |
|
|
||
| .status-color-not-reported { | ||
| color: #ec7a08; | ||
| } |
There was a problem hiding this comment.
While I understand the whole direction where Foreman core is going, I am not sold on introducing the whole JS/CSS stack into this plugin because of four extracted inline styles. It does not make much sense, discovery has always been quite UX small, four screens actually in total, we are unlikely adding dozen of new screens in this plugin. It was doing fine with the four styles and two JS functions inline.
I am not familiar with these technologies myself, but I do most of reviews, all the release engineering, packaging and troubleshooting upstream and downstream and these four lines can make my life more tough. Therefore I'd like to get volunteers from @theforeman/discovery maintainers for active involvement in packaging and related issues. @ares
There was a problem hiding this comment.
@lzap I understand your concern, these changes won't look any different if we would move from inline to the traditional rails asset pipeline.
TBH: using inline style is a hack, and we should try to allow great UX experience, maintainability of the code is one of the first steps towards that. I don't think that packaging webpack assets is any different at this stage from any other plugin...
There was a problem hiding this comment.
@lzap the "inline" was a pain even to you.
For example the code had stuff like `<% th_style = "color: black ..." %> and reused it in many locations.
On email based HTML it can pass, because it's problematic to write good HTML styled for emails, but using an external styling actually does this in a more web concept rather then using tempting to do the same.
I don't think that helpers or templates should have to "know" what does a color, icon etc should be, but rather what class to use.
The reason is, that it's easier to change style without touching the Ruby/Template constantly, unless there is a change to the class name.
For SCSS, it's a macro language that transfer into CSS rules. It contains variables (that just started to appear in CSS itself), it contains mixin for not repeating the same rule, it contains the ability to create nested rules, and create a "story" in that language to explain the styling for all levels, and more.
And then the language is transpiled into CSS rules, and the macros translated to actual rules, the variables are like place holders etc...
| @@ -0,0 +1,48 @@ | |||
| { | |||
There was a problem hiding this comment.
technically - for css you dont need eslitnrc, but it maybe ok to leave here.
| <td style="<%= td_style%>"><%= number_to_human_size(discovery_attribute(host, :memory, 0) * 1024 * 1024) %></td> | ||
| <td style="<%= td_style%>"><%= discovery_attribute(host, :disk_count) %></td> | ||
| <td style="<%= td_style%>"><%= number_to_human_size(discovery_attribute(host, :disks_size, 0) * 1024 * 1024) %></td> | ||
| <td class="discovery-mailer-td-style"><%= host.try(:hardware_model_name) || 'N/A' %></td> |
There was a problem hiding this comment.
can't we find a better css matcher? you can probably take any td inside a discovery table or something like that?
There was a problem hiding this comment.
I was thinking about bem, but didn't want to go all the way, because nothing works in that manner here, so even the name convention is not bem, but hints for it.
I do understand why to choose id instead of class for one element, but if I understand correctly the design, each cell might contain different styling, so I didn't want to break that design.
Is my understanding wrong?
| @@ -4,23 +4,68 @@ | |||
| <head> | |||
| <meta content="text/html; charset=UTF-8" http-equiv="Content-Type" /> | |||
| <title><%= _('Summary report for discovered hosts from Foreman') %></title> | |||
| <style> | |||
There was a problem hiding this comment.
core uses a different approach using the roadie-rails gem. if you don't want to do it as part of this pr, please revert these changes?
There was a problem hiding this comment.
missed that, sorry, I'm fixing it
There was a problem hiding this comment.
Add the change, did not test it yet, I will notify when it is ready
|
|
||
| .status-color-not-reported { | ||
| color: #ec7a08; | ||
| } |
There was a problem hiding this comment.
@lzap I understand your concern, these changes won't look any different if we would move from inline to the traditional rails asset pipeline.
TBH: using inline style is a hack, and we should try to allow great UX experience, maintainability of the code is one of the first steps towards that. I don't think that packaging webpack assets is any different at this stage from any other plugin...
theforeman-bot
added
Waiting on contributor
Needs re-review
Needs testing
and removed
Not yet reviewed
Waiting on contributor
labels
app/assets/stylesheets/summary.css
Outdated
| @@ -0,0 +1,46 @@ | |||
| body { | |||
There was a problem hiding this comment.
What is the reason to put this css here and not in webpack?
|
Testing the current plugin, trying to figure out few things, actually
looking to see how to include it in webpack, but load it only for the email
and not as a link, but rather then a content. So it's for test purpose only.
…On Thu, May 3, 2018 at 1:31 PM, Avi Sharvit ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/assets/stylesheets/summary.css
<#421 (comment)>
:
> @@ -0,0 +1,46 @@
+body {
What is the reason to put this css here and not in webpack?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#421 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEdg_34yWdeZbEgcNJHgpb-ekBs6dxvks5tutx0gaJpZM4TPoaI>
.
|
|
two issues:
|
| @@ -4,23 +4,22 @@ | |||
| <head> | |||
| <meta content="text/html; charset=UTF-8" http-equiv="Content-Type" /> | |||
| <title><%= _('Summary report for discovered hosts from Foreman') %></title> | |||
| <%= stylesheet_link_tag 'email_summary.css', 'data-turbolinks-track': true %> | |||
There was a problem hiding this comment.
Can you try it without the .css?
Do you actually need turbokinks? isn't it an email template?
<%= stylesheet_link_tag 'email_summary' %>There was a problem hiding this comment.
Removed them both, and still it is not part of the pipeline :(
|
Try moving it to |
|
now it finds the css, unable to test it yet by hand, working on it So please DO NOT MERGE YET |
|
It works :) |
|
You say "stage 1", can you describe all stages, what is the plan? Please include detailed packaging changes required for this to work on RPM/DEB installs. |
|
@lzap the discussion here actually changed from "stage1" to the only stage. |
|
Ok, thanks. I need to know what the packaging necessary changes are. If I merge this, will packaging break? If so, let's do package changes first. |
|
I'm not sure that I know, that is I know what components I'm using, but not how packaging works for them regarding the way foreman package things. @ekohl for asset pipeline, and for webpack, what is required for packaging purpose? |
|
FWIW, the last time I looked into formatting in HTML mails, some clients didn't support any non-inline styling. This may have changed by now, since it was about 4 years ago, but this should be tested to make sure the formatting works well on at least some common clients (such as Outlook for example) |
In theforeman/foreman-packaging@2655ffb we added it to the template. You can now regenerate a spec file with add_gem_package.sh and if the gem contains the webpack directory it should work. The only thing we haven't looked into is additional nodejs dependencies that foreman doesn't have. |
|
@tbrisker the email plugin that is used actually takes the asset pipe line and placed it literally as inline css for each class element that exists, so you'll find there also @lzap based on @ekohl's replay, there is no need at this time for anything special and it will take it all. For JS at this time we do not have anything special at all. |
|
@lzap is correct: we do need to modify the packaging but it's well known which changes need to be made and it should be easy to do. |
|
Ok, I am gonna test and merge this after release of discovery for 1.18 foreman, just to be sure we have enough time for fixes. I am not confident in this area. Ping me if I forget after the release. |
|
@lzap, please allow me first to finish understanding how to package it (it's not just the release but also a learning curve for me), so for next requirement it will be faster, but not necessarily simpler. |
|
No rush, I will be branching off Discovery next week. |
|
Given the huge difficulties webpack introduces in plugins I'd advise any plugin author to hold it off until it stabilizes. This also mixes 2 asset compilation stacks. Is that intentional? |
|
@ekohl the two assets are intentional. The asset pipline, is actually for email, we are using email library that takes the classes names and place at the email body the actual style based on that. |
|
Let's revisit this post 1.19 then. |
|
Discovery develop is finally green again, please rebase to retest on jenkins. |
|
@ik5 this seems really close, would you like to solve the conflicts and rebase? |
This stage contains the "bootstrap" for all the node stack, nothing more