Remove inline styles #456
Remove inline styles #456
Conversation
There was a problem hiding this comment.
@liza-pizza thanks so much for this PR 😁 I haven't gotten around to fully reviewing it yet - but first off, could you format the less when you get a chance? Usually I do this with the atom beautify plugin (since I use atom as my text editor), but any other text editor auto-tidy plugin would be fine too.
Edit to add: by "format" I mean make the indentation all uniform, e.g.
// not great
.someClass {
color: purple;
.childClass{background-color:yellow}
}
//better
.someClass {
color: purple;
.childClass {
background-color:yellow;
}
}Let me know if that isn't clear!
|
I hope this is enough for now. Do let me know if there are any errors to correct. |
There was a problem hiding this comment.
Thanks for working on this! It's always tricky to get into a new CSS codebase, especially one where we use a different syntax (hiccup) for HTML.
There are some selectors which could be more specific to the element, instead of global.
Some selectors also missed edge cases which you probably didn't notice.
It would be great if you could look into my comments.
| @@ -46,8 +46,7 @@ | |||
| (not (contains? registry current-mine)) | |||
| {:db (assoc db-with-registry :current-mine :default) | |||
| :dispatch [:messages/add | |||
| {:markup [:span (str "Your mine has been changed to the default as your selected mine '" (name current-mine) "' was not present in the registry.")] | |||
| :style "warning"}]} | |||
There was a problem hiding this comment.
Could you revert this change? It's by chance called :style but not an inline style in this instance.
| @@ -140,3 +140,8 @@ input.form-control:focus { | |||
| label { | |||
| color: @body-foreground-color; | |||
| } | |||
|
|
|||
| .col-xs-3 { | |||
There was a problem hiding this comment.
I think we should have something more specific here, probably in enrichment.less instead.
If you can't find a good way to target the element, coming up with a new suitable class name for the element is fine as well.
| @@ -38,8 +38,7 @@ | |||
| [:span.markup markup] | |||
| [:span.controls | |||
| [:button.btn.btn-default.btn-xs.btn-raised | |||
| {:on-click (fn [] (dispatch [:messages/remove id])) | |||
| :style {:margin 0}} "X"]]])})) | |||
There was a problem hiding this comment.
I can't see that margin: 0 is added back via the CSS, and in this case it makes a visual difference.
Could you add it to alerts.less?
| @@ -43,6 +43,7 @@ | |||
| .icon { | |||
| color: @cta-color-text; | |||
| fill: @cta-color-text; | |||
| position: absolute; | |||
There was a problem hiding this comment.
This seems to break the positioning of icons for some buttons elsewhere.
Could you instead add a more specific rule to templates.less, under .constraint-container?
| @@ -353,6 +354,11 @@ button.btn, button.cta { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I don't think many of these selectors added to site.less make much sense as global styles.
Each "component" generally have their own less source file. In the case of .container-fluid.results, you could add this styling to the less/components/results.less file instead.
Could you go through the selectors you added to site.less and see if there exist a more suitable less file to place them? There's usually a top-level class name that these page-specific styles will be nested under. Don't hesitate to create new class names if you feel this is necessary.
| @@ -174,6 +174,10 @@ | |||
|
|
|||
| div.value {} | |||
| } | |||
|
|
|||
| .col-xs-2 { | |||
There was a problem hiding this comment.
This is too specific. What if we changed the markup to use .col-xs-3 instead? We would still want the rule to work.
| @@ -118,6 +118,11 @@ | |||
| .allresults { | |||
| display: flex; | |||
| flex-direction: column; | |||
|
|
|||
| .feature { | |||
There was a problem hiding this comment.
Excellent, this is what I want to see more of [=
| @@ -395,6 +397,7 @@ div.column-container { | |||
| .qb-label { | |||
| padding-left: 0; | |||
| white-space: nowrap; | |||
| margin-left: 5px; | |||
There was a problem hiding this comment.
.qb-label is also used for query builder attributes, where we don't want the margin-left (see src/cljs/bluegenes/pages/querybuilder/views.cljs:83).
Maybe split it into .qb-attribute-label where the margin-left is the only difference, changing the attributes to use this class instead?
| } | ||
|
|
||
|
|
||
| .icon.icon-globe { |
There was a problem hiding this comment.
I also feel this is too general. Try moving this into mymine.less to target that use of icon-globe specifically.
Removes all inline styles that can be written into the less files instead. There is the exception of those in idresolver.views and template.views namespaces but ,if I am correct ,they cannot be built into less files.
This resolves #316