Skip to content

Remove inline styles #456

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.

Sign up for GitHub

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

Open
liza-pizza wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Remove inline styles #456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1e6e612
remove inline styles for icons and add to less files
liza-pizza Dec 14, 2019
3adbdd7
remove inline styles from dropdown menu
liza-pizza Dec 20, 2019
19806c8
remove remaining inline styles and add to less files
liza-pizza Dec 23, 2019
e516ede
correct icon style
liza-pizza Dec 25, 2019
367388a
remove final inline styles
liza-pizza Dec 25, 2019
65a0b3b
format less files
liza-pizza Jan 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions less/components/enrichment.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@

div.value {}
}

.col-xs-2 {
Copy link
Copy Markdown
Member

@heralden heralden Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too specific. What if we changed the markup to use .col-xs-3 instead? We would still want the rule to work.

white-space: nowrap;
}
}

.popover-content {
Expand Down
12 changes: 12 additions & 0 deletions less/components/icons.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,15 @@
.icon-2x {
.icon(2em)
}


#icons {
position: absolute;
width: 0;
height: 0;
}


.icon.icon-globe {
Copy link
Copy Markdown
Member

@heralden heralden Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also feel this is too general. Try moving this into mymine.less to target that use of icon-globe specifically.

fill: #939393;
}
4 changes: 4 additions & 0 deletions less/components/querybuilder.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ div.column-container {
}

.model-browser {

.icon {
fill: darken(silver,10%);
}
Expand Down Expand Up @@ -308,6 +309,7 @@ div.column-container {
// padding-left: 0.2em;
border: 1px solid #d5d5d5;
border-width: 0 0 1px 1px;
white-space: nowrap;

&.haschildren {
border-bottom: 0;
Expand Down Expand Up @@ -395,6 +397,7 @@ div.column-container {
.qb-label {
padding-left: 0;
white-space: nowrap;
margin-left: 5px;
Copy link
Copy Markdown
Member

@heralden heralden Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.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-blank::before,
Expand Down Expand Up @@ -549,6 +552,7 @@ div.column-container {
background-color: #c8ced0;
color: #666;
border-radius: 3px;
margin-left: 5px;
}

.label-no-results {
Expand Down
5 changes: 5 additions & 0 deletions less/components/regions.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@
.allresults {
display: flex;
flex-direction: column;

.feature {
Copy link
Copy Markdown
Member

@heralden heralden Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, this is what I want to see more of [=

overflow-wrap: break-word;
}

}

.results {
Expand Down
6 changes: 3 additions & 3 deletions less/im-tables-overrides.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,9 @@
min-height: 2em;
}

.im-table-toolbar label {
.dark-text;
}
.im-table-toolbar label {
.dark-text;
}
}

.results-and-enrichment {
Expand Down
5 changes: 5 additions & 0 deletions less/overrides.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,8 @@ input.form-control:focus {
label {
color: @body-foreground-color;
}

.col-xs-3 {
Copy link
Copy Markdown
Member

@heralden heralden Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

font-size: 0.8em;
}

23 changes: 23 additions & 0 deletions less/site.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ label {
margin-right: 0;
}


.loading {
display: flex;
justify-content: center;
Expand Down Expand Up @@ -353,6 +354,11 @@ button.btn, button.cta {
}
}

Copy link
Copy Markdown
Member

@heralden heralden Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.btn.btn-raised.dropdown-toggle {
text-transform: none;
white-space: normal;
}

.dropdown-mixed-content {
//mixin used for list / lookup/ = / > / < lookup dropdown
//(probably other places too)
Expand Down Expand Up @@ -399,13 +405,30 @@ button.btn, button.cta {
}
}

.container-fluid.results {
width: 100%;
}

.icon.icon-document-list {
margin-left: 0px;
}

.relative.im-table {
background-color: white;
}

.no-gutter {
margin-right: 0;
margin-left: 0;
}

.description-div {
background-color: #D2CEBF;
padding: 10px;
overflow: auto;
}


.no-gutter > [class*="col-"] {
padding-right: 5px;
padding-left: 5px;
Expand Down
1 change: 1 addition & 0 deletions less/variables.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
.icon {
color: @cta-color-text;
fill: @cta-color-text;
position: absolute;
Copy link
Copy Markdown
Member

@heralden heralden Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}

Expand Down
15 changes: 6 additions & 9 deletions src/cljs/bluegenes/components/icons.cljs
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 1 addition & 2 deletions src/cljs/bluegenes/components/ui/alerts.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]]])}))
Copy link
Copy Markdown
Member

@heralden heralden Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{:on-click (fn [] (dispatch [:messages/remove id]))} "X"]]])}))

(defn messages
"Creates a message bar on the bottom of the screen"
Expand Down
4 changes: 1 addition & 3 deletions src/cljs/bluegenes/components/ui/constraint.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,6 @@
((or on-change-operator on-change) {:code code :path path :value (cond-> value (seq? value) first) :op op})))]]
[:div.col-sm-8
[:div
{:style {:position "relative"}}
[constraint-text-input
:model model
:value value
Expand All @@ -276,8 +275,7 @@
((or on-blur on-change) {:path path :value val :op op :code code})))]
(when on-remove
[:svg.icon.icon-bin
{:style {:position "absolute"}
:on-click (fn [op] (on-remove {:path path :value value :op op}))}
{:on-click (fn [op] (on-remove {:path path :value value :op op}))}
[:use {:xlinkHref "#icon-bin"}]])]]]]
#_[:div.constraint-component
[:div.input-group.constraint-input
Expand Down
6 changes: 2 additions & 4 deletions src/cljs/bluegenes/components/ui/list_dropdown.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,16 @@
filter-fn (apply every-pred [text-filter type-filter])
filtered-lists (filter filter-fn lists)]
[:div.dropdown
[:button.btn.btn-raised.btn-default.dropdown-toggle
[:button.btn.btn-raised.dropdown-toggle
{:disabled disabled
:style {:text-transform "none"
:white-space "normal"}
:data-toggle "dropdown"}
(str (or value "Choose a list") " ") [:span.caret]]
[:div.dropdown-menu.dropdown-mixed-content
(if (some? (not-empty lists))
[:div.container-fluid
[text-filter-form text-filter-atom]
[:div.col-md-6
[:h4 [:svg.icon.icon-history [:use {:xlinkHref "#icon-history"}]] " Recently Created"]
[:h4 [:svg.icon.icon-history [:use {:xlinkHref "#icon-history"}]] " Recently Created"]
[im-lists
:lists (take 5 (sort-by :timestamp filtered-lists))
:on-change on-change]]
Expand Down
3 changes: 1 addition & 2 deletions src/cljs/bluegenes/events/registry.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"}]}
Copy link
Copy Markdown
Member

@heralden heralden Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you revert this change? It's by chance called :style but not an inline style in this instance.

{:markup [:span (str "Your mine has been changed to the default as your selected mine '" (name current-mine) "' was not present in the registry.")]}]}
;; Fill in the mine details if it's missing.
;; (This happens when we use a registry mine.)
(nil? (get-in db-with-registry [:mines current-mine]))
Expand Down
5 changes: 3 additions & 2 deletions src/cljs/bluegenes/pages/mymine/views/mymine.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@
(defn ico []
(fn [file-type]
(case file-type
"list" [:svg.icon.icon-document-list {:style {:margin-left 0}}
"list" [:svg.icon.icon-document-list
[:use {:xlinkHref "#icon-document-list"}]]
"tag" [:svg.icon.icon-folder [:use {:xlinkHref "#icon-folder"}]]
[:svg.icon.icon-folder [:use {:xlinkHref "#icon-spyglass"}]])))
Expand All @@ -146,7 +146,8 @@
[:a {:href (route/href ::route/list {:title (:title dets)})}
name]
(when-not authorized
[:svg.icon.icon-globe {:style {:fill "#939393"}} [:use {:xlinkHref "#icon-globe"}]])]
[:svg.icon.icon-globe
[:use {:xlinkHref "#icon-globe"}]])]
[:div.description-text
description]]
[:div.col-1 {:class (str "tag-type type-" type)} type]
Expand Down
15 changes: 6 additions & 9 deletions src/cljs/bluegenes/pages/querybuilder/views.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,10 @@
[:div.model-browser
(let [path [root-class]]
(into [:ul
[:li [:div
{:style {:white-space "nowrap"}}
[:button.btn.btn-default.btn-slim
{:on-click (fn [] (dispatch [:qb/enhance-query-add-summary-views [root-class]]))}
[:span.label-button
"Summary"]]
[:li [:div [:button.btn.btn-default.btn-slim
{:on-click (fn [] (dispatch [:qb/enhance-query-add-summary-views [root-class]]))}
[:span.label-button
"Summary"]]
[:button.btn.btn-default.btn-slim
{:on-click (fn [] (dispatch [:qb/expand-all]))}
"Expand to Selection"]
Expand All @@ -155,7 +153,7 @@
[:li.tree.haschildren
[:div.flexmex
[:span.lab {:class (if (im-path/class? model (join "." path)) "qb-class" "qb-attribute")}
[:span.qb-label {:style {:margin-left 5}} [tooltip {:on-click #(dispatch [:qb/expand-path path]) :title (str "Show " (uncamel k) " in the model browser")} [:a (uncamel k)]]]
[:span.qb-label [tooltip {:on-click #(dispatch [:qb/expand-path path]) :title (str "Show " (uncamel k) " in the model browser")} [:a (uncamel k)]]]
(when-let [s (:subclass properties)] [:span.label.label-default (uncamel s)])
[:svg.icon.icon-bin
{:on-click (if (> (count path) 1)
Expand All @@ -166,8 +164,7 @@
[:svg.icon.icon-filter {:on-click (fn [] (dispatch [:qb/enhance-query-add-constraint path]))} [:use {:xlinkHref "#icon-filter"}]]
(when-let [c (:id-count properties)]
[:span.label.label-soft
{:class (when (= 0 c) "label-no-results")
:style {:margin-left 5}} (str c " row" (when (not= c 1) "s"))])]]
{:class (when (= 0 c) "label-no-results")} (str c " row" (when (not= c 1) "s"))])]]
(when-let [constraints (:constraints properties)]
(into [:ul.tree.banana]
(map-indexed (fn [idx con]
Expand Down
3 changes: 1 addition & 2 deletions src/cljs/bluegenes/pages/regions/results.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@
:type class
:id objectId})}
[:div.grid-3_xs-3.single-feature
[:div.col {:style {:word-wrap "break-word"}}
primaryIdentifier]
[:div.col.feature primaryIdentifier]
[:div.col the-type]
[:div.col (str
(get-in chromosomeLocation [:locatedOn :primaryIdentifier])
Expand Down
3 changes: 1 addition & 2 deletions src/cljs/bluegenes/pages/reportpage/views.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@
[:h3 (str title (when result-count (str " (" result-count ")")))]
(if (= 0 result-count)
[:div "No Results"]
[:div {:style {:background-color "white"}}
[im-table/main loc]])])))})))
[:div [im-table/main loc]])])))})))

(defn strip-class [s]
(clojure.string/join "." (drop 1 (clojure.string/split s "."))))
Expand Down
4 changes: 2 additions & 2 deletions src/cljs/bluegenes/pages/results/enrichment/views.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@
:title
identifier)}]))}
description]}])]
[:div.col-xs-3 [:span {:style {:font-size "0.8em"}} (.toExponential p-value 6)]]]]])))
[:div.col-xs-3 [:span
(.toExponential p-value 6)]]]]])))

(defn has-text?
"Return true if a label contains a string"
Expand All @@ -94,7 +95,6 @@
[:div.container-fluid
[:div.row
[:div.col-xs-2
{:style {:white-space "nowrap"}}
[:input
{:type "checkbox"
:checked selected?
Expand Down
3 changes: 0 additions & 3 deletions src/cljs/bluegenes/pages/results/views.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,14 @@
(if @are-there-results?
;;show results
[:div.container-fluid.results
{:style {:width "100%"}}
[:div.row
[:div.col-sm-2
[query-history]]
[:div.col-sm-7
[:div
{:style {:background-color "white"}}
[tables/main [:results :table]]
(when (> (count description) 0)
[:div.description-div
{:style {:background-color "#D2CEBF" :padding "10px" :overflow "auto"}}
[:b "Description: "]
description])]
[:div
Expand Down