Skip to content
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

DATAUP-246: CSS cleanup, pt II #1879

Merged
merged 2 commits into from
Oct 22, 2020
Merged

Conversation

ialarmedalien
Copy link
Collaborator

@ialarmedalien ialarmedalien commented Oct 19, 2020

NOTE: this is built on top of the whitespace changes made in #1872. When #1872 is merged, I will rebase this PR.

Description of PR purpose/changes

The changes made in this PR include:

  • removing commented-out styles
  • removing invalid css tags and values
  • removing duplicated stanzas
  • combining declarations spread over several lines

Jira Ticket / Issue

https://kbase-jira.atlassian.net/browse/DATAUP-246

Testing Instructions

  • Details for how to test the PR:
  • Tests pass in travis and locally
  • Changes available by spinning up a local narrative and navigating around to check everything looks reasonable

Dev Checklist:

  • My code follows the guidelines at https://sites.google.com/truss.works/kbasetruss/development
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

@@ -105,17 +99,14 @@

.kb-app-cell .kb-app-parameter-row .message.-error {
background: #f2dede;
color: #f44336;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved from line 115

@@ -188,6 +169,7 @@ control with padding, and then scooting the icon back into its column.
color: #777;
text-align: left;
margin-top: 3px;
padding-left: 7px;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved from line 179

Comment on lines -246 to -249
.kb-app-cell .kb-app-subtitle {
background-color: #f5f5f5;
padding: 3px 5px;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

duplicate stanza

Comment on lines -436 to -441
.btn.kb-app-cell-btn.btn-danger.active:hover {
color: rgb(209, 82, 65);
background-color: #ddd;
border-bottom: 2px rgb(209, 82, 65) solid;
margin-bottom: 4px;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

duplicate stanza - see line 415 in old version / 392 in new version

@@ -77,7 +77,6 @@
}

.panel.kb-panel-light .panel-body {
padding-top: 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

overridden in next line

Comment on lines -20 to -23
/* Set a plain default color -- should be overridden when building the button. */
.btn.kb-flat-btn {
color: #666;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

combined into stanza starting at line 4

Comment on lines -38 to -40
.btn.kb-flat-btn:hover {
background-color: #f5f5f5;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

overridden by stanza starting @ line 50 (line 22 in new version)

Comment on lines -61 to -62
text-decoration-style: solid;
text-decoration-color: red;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not valid css

Comment on lines -68 to -70
#kblog-msg {
margin-top: 5px;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

combined with next stanza

Comment on lines -371 to -376
.view-header {
font: 1.5em/1.75em 'RobotoBlack', Arial, sans-serif;
font-weight: normal;
color: #898989;
letter-spacing: 0;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

duplicates the previous stanza

Comment on lines +98 to +103
-moz-transition: box-shadow 0.2s ease-in-out;
-o-transition: box-shadow 0.2s ease-in-out;
transition: box-shadow 0.2s ease-in-out;
-webkit-box-shadow: 0 1px 2px #aaa, 0 5px 5px #aaa;
-moz-box-shadow: 0 1px 2px #aaa, 0 5px 5px #aaa;
box-shadow: 0 1px 2px #aaa, 0 5px 5px #aaa;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reordering as the unprefixed version should go last

Comment on lines +125 to +133
#notebook {
padding: 0;
}

span#notebook_name {
#notebook_name {
color: #006698;
}

span#notebook_name:hover {
#notebook_name:hover {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should not require a tag if there is an ID

@@ -165,7 +164,7 @@ span#notebook_name:hover {

.btn-toolbar {
margin-left: 0;
padding-left: -5px;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cannot have negative values for padding

@@ -128,21 +128,21 @@ body {

strong,
b {
font-family: 'Oxygen';
font-family: 'Oxygen', Arial, sans-serif;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding in fallbacks

@@ -7,10 +7,6 @@
z-index: 1000;
}

body {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved into body stanza at line 27 in new version

@@ -23,56 +19,40 @@ body {
}

html {
background-color: #a2a2a2;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Overridden at (old) line 31

box-shadow: none;
moz-box-shadow: none;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

invalid -- converted to -moz-box-shadow throughout

@@ -585,7 +514,6 @@ img.dataset-cell {
background-image: url("../images/gears.png");
background-position: right;
background-repeat: no-repeat;
#background-color: #f2f2f2;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

invalid syntax for css

@@ -682,7 +610,6 @@ div.metadata {

div.social {
float: right;
display: block;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

overridden three lines later

@@ -696,7 +623,7 @@ p span.notification {
}

/**
* Bootstrap 3 submenu hack^H^H^H^H fix
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bye bye ^Hs

Comment on lines -778 to -780
#menubar .navbar {
margin-bottom: 5px;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

combined with #menubar .navbar at (old) line 833

@@ -80,13 +80,13 @@
line-height: 18px;
text-align: center;
text-transform: uppercase;
color: #4379B1;
color: #4379b1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

stylelint, the css formatter I have been using, lowercases all the colour lettering.

Comment on lines -217 to -219
}

.kb-data-list-drag-target.-drag-hover {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

combining stanzas

-webkit-box-shadow: none;
moz-box-shadow: none;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

invalid - corrected to -moz-box-shadow throughout

-webkit-box-shadow: none;
moz-box-shadow: none;
xfont-size: 18px;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

invalid

@@ -1075,7 +1068,6 @@ div.metadata {

div.social {
float: right;
display: block;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

overridden a couple of lines later

Comment on lines -1171 to -1173
#menubar .navbar {
margin-bottom: 5px;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

combine with stanza at (old) line 1209 / (new) line 1197

Comment on lines -2011 to -2014
.kb-method-parameter-hint {
padding-left: 7px;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

combine with stanza at (old) line 2019 / (new) line 1992

Comment on lines -2629 to -2631

--webkit-box-shadow: 1px 1px 1px 1px #fff;
--moz-box-shadow: 1px 1px 1px 1px #fff;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

another variation on the theme of "wrong"

@@ -3241,8 +3191,7 @@ button.kb-data-obj {
}

.kb-nar-manager-titles {
font-family: "Oxygen";
font-weight: bold;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

repeated at line 3250

Comment on lines -3374 to -3376
.kb-import-info {
font-weight: normal;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

combine with stanza at (old) line 3363 / (new) line 3302

- removing commented-out styles
- removing invalid css tags and values
- removing duplicated stanzas
- combining declarations spread over several lines
@ialarmedalien ialarmedalien force-pushed the DATAUP-246_css_to_sass_css_clean_up branch from 50909a4 to 0c43d4e Compare October 19, 2020 19:12
Copy link
Member

@briehl briehl left a comment

Choose a reason for hiding this comment

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

Just had a small comment, but this is great!

kbase-extension/static/kbase/css/narrative.css Outdated Show resolved Hide resolved
Copy link
Contributor

@leia-sefkin leia-sefkin left a comment

Choose a reason for hiding this comment

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

looks good overall - thanks for this it's a massive improvement

Base automatically changed from DATAUP-246_css_to_sass-whitespace to truss October 20, 2020 16:13
@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2020

SonarCloud Quality Gate failed.

Bug D 9 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@eamahanna eamahanna left a comment

Choose a reason for hiding this comment

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

All of the changes look good, once ci/narrative refactor are up and running I will look at the changes locally and do a spot check to make sure everything still looks/functions the same. After that, I am happy to approve :)

Copy link
Contributor

@eamahanna eamahanna left a comment

Choose a reason for hiding this comment

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

Also, there are a few errors and warnings in codacy and sonar that may need to be addressed.

@ialarmedalien
Copy link
Collaborator Author

@eamahanna thanks for the review! I am going to address the codacy/sonarcloud issues in another PR; trying to keep these PRs to a single type of fix to make it easier for reviewers. This one is just removing comments, invalid CSS, and duplicated code. Next PR (or PR after that) will fix some of the codacy issues, like not having fallbacks for font declarations.

Copy link
Member

@briehl briehl left a comment

Choose a reason for hiding this comment

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

Looks great! I think we should merge.

@briehl briehl merged commit beef10e into truss Oct 22, 2020
@briehl briehl deleted the DATAUP-246_css_to_sass_css_clean_up branch October 22, 2020 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants