mgr/dashboard: Color variables for color codes #22695

a2batic · 2018-06-25T20:41:28Z

Assign variables to colors present in css files
Changed navigation icons padding because the notification dropdown was going out of window.
Fixes: http://tracker.ceph.com/issues/24575
Signed-off-by: Kanika Murarka kmurarka@redhat.com

tspmelo

Now that we have a centralized file for color, I notice that we have a lot of whites and greys that look the same.

Would it be possible to reduce that? maybe 3 of each?

tspmelo · 2018-06-25T21:31:03Z

src/pybind/mgr/dashboard/frontend/src/app/ceph/dashboard/health/health.component.scss

@@ -1,3 +1,5 @@
+@import '../../../../defaults';


This import is not being used.

Ack, will remove it.

tspmelo · 2018-06-25T22:37:03Z

src/pybind/mgr/dashboard/frontend/src/openattic-theme.scss

@@ -30,11 +30,6 @@
  CSS Fix
 */



I think the colors in this file should also be extracted.

tspmelo · 2018-06-25T22:41:15Z

src/pybind/mgr/dashboard/frontend/src/app/shared/datatable/table/table.component.scss

@@ -63,7 +63,7 @@
  }
  .input-group {
    float: right;
-    border-left: 1px solid rgba(0,0,0,.09);
+    border-left: $border-color-white;


Should we extract the whole rule or just the color?
Do you think we will need to change the border size and texture when customizing the template?
If we extracted just the color we might be able to reduce the number of color variables.

Ack, it is better to assign variable just to color.

a2batic · 2018-06-26T17:48:20Z

@tspmelo, ack, will reduce the colors by eliminating similar shades.

votdev · 2018-06-27T07:36:57Z

src/pybind/mgr/dashboard/frontend/src/defaults.scss

+/*Table Component*/
+$color-off-white: #f5f5f5;
+$color-mild-white: #f6f6f6;
+$border-color-white: 1px solid rgba(0,0,0,.09);


I think you should split that and set the border style in the table component scss.

This is nice step to put all color literals into a single place, but pushing things a bit further: what about using a color naming scheme that describe the meaning/intention of the element colored instead of simply describing the color shade?

Let me put an example: instead of color-solid-red, what about color-health-error?

Sounds good to me. But i would like to see something like this to make it really really simple:

$color-solid-red: #FF0000; $color-health-error: $color-solid-red;

votdev · 2018-06-27T07:37:03Z

src/pybind/mgr/dashboard/frontend/src/defaults.scss

+$color-light-white: #f9f9f9;
+$color-solid-white: #fafafa;
+$color-gallery-gray: #ededed;
+$border-color-gray: 1px solid #d1d1d1;


I think you should split that and set the border style in the table component scss.

callithea · 2018-07-05T08:37:02Z

jenkins test make check

epuertat · 2018-07-05T11:33:59Z

src/pybind/mgr/dashboard/frontend/src/app/ceph/dashboard/health/health.component.ts

@@ -62,26 +64,25 @@ export class HealthComponent implements OnInit, OnDestroy {

  preparePoolUsage(chart, data) {
    const colors = [


Why not using a colour generator/random colours? Even better if it copes with color blindness (8% population affecting).

I've seen that the AngularJS ChartJS module includes a random generator.

A solutions based on palette.js and another.

And here you can find a discussion on this.

Thanks, I will take a look.

epuertat · 2018-07-05T11:34:47Z

src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-user-form/rgw-user-form.component.scss

@@ -0,0 +1,4 @@
+form .input-group-addon {
+  color: #a2a2a2 !important;


Why adding a literal color here?

I forgot to remove it. Will replace it.

epuertat

Nice job. Looks almost perfect. Just a few comments and the colour palette generator suggestion. Thanks.

epuertat · 2018-07-05T11:37:40Z

src/pybind/mgr/dashboard/frontend/src/app/core/navigation/navigation.module.scss

@@ -0,0 +1,195 @@
+/* Branding */
+.navbar-openattic {


Since you're removing openattic specific namings, why leaving this here?

Ack, will change this.

epuertat · 2018-07-05T11:39:16Z

src/pybind/mgr/dashboard/frontend/src/openattic-theme.scss

@@ -1,1144 +0,0 @@
-/*


Have you removed this file?

I have removed this file and copied all important and global classes and styles to styles.scss file.

a2batic · 2018-07-10T07:28:10Z

The doughnut chart now looks this this :

epuertat

Please just check my comments, but this looks overall good to me!

epuertat · 2018-07-10T09:25:06Z

src/pybind/mgr/dashboard/frontend/src/styles.scss

+@import 'defaults';
+$fa-font-path: "../node_modules/fork-awesome/fonts";
+@import "../node_modules/fork-awesome/scss/fork-awesome";
+@import 'src/app/core/navigation/navigation.module';


I'd try to either move that CSS code back here or leave it as a component CSS one, but not both ways, as I find it might be confusing. Try the suggestion by @alfonsomthd to change the view encapsulation.

It's not common practice to create .scss files for modules.

If there is no parent component where you can add this rules, I would suggest you do what we did for the popover styles: create a styles/navigation.scss file and import it in every component you need it.

epuertat · 2018-07-10T09:28:14Z

src/pybind/mgr/dashboard/frontend/angular.json

-              "node_modules/chart.js/dist/Chart.bundle.js"
+              "node_modules/angular/angular.js",
+              "node_modules/chart.js/dist/Chart.bundle.js",
+              "node_modules/angular-chart.js/dist/angular-chart.js"


Let's go with this one and we then may try with a more advanced color schemer.

tspmelo · 2018-07-10T18:34:13Z

src/pybind/mgr/dashboard/frontend/package.json

@@ -69,6 +69,7 @@
    "@types/jasminewd2": "2.0.3",
    "@types/jest": "23.1.2",
    "@types/node": "8.10.20",
+    "angular-chart.js": "^1.1.1",


Please be careful when adding new dependencies.

This package is targetted at Angular.js and is not supported by the current version of Angular.

We already have a package that wraps Chart.js for Angular, ng2-charts.

When adding a new dependency please remove the ^ and ~ from the version.

Since this package would have been used in the final version of the dashboard, it should have been added to the dependencies section, the devDependencies section is specific for packages that are used during the dev steps.

tspmelo · 2018-07-10T18:34:19Z

src/pybind/mgr/dashboard/frontend/angular.json

-              "node_modules/chart.js/dist/Chart.bundle.js"
+              "node_modules/angular/angular.js",
+              "node_modules/chart.js/dist/Chart.bundle.js",
+              "node_modules/angular-chart.js/dist/angular-chart.js"


This should be reverted.
Please read the reason in my previous comment.

tspmelo · 2018-07-10T18:35:08Z

src/pybind/mgr/dashboard/frontend/src/app/ceph/dashboard/health-pie/health-pie.component.html

@@ -1,11 +1,11 @@
 <div class="chart-container">
  <canvas baseChart
          #chartCanvas
+          class="chart chart-doughnut"


This can be removed. We are already defining the chart using the chartType attribute.

tspmelo · 2018-07-10T18:39:18Z

src/pybind/mgr/dashboard/frontend/src/app/core/navigation/navigation.module.scss

+
+  /* Navbar */
+  margin-bottom: 0;
+  background: $color-navbar-bg;


It seems @import '../../../../defaults'; is missing.

tspmelo · 2018-07-10T18:42:49Z

src/pybind/mgr/dashboard/frontend/src/app/shared/datatable/table/table.component.scss

-    background-image: -webkit-linear-gradient(top,#fafafa 0,#ededed 100%);
-    background-image: -o-linear-gradient(top,#fafafa 0,#ededed 100%);
-    background-image: linear-gradient(to bottom,#fafafa 0,#ededed 100%);
+    background-color: red;


Should we use a variable here?

tspmelo · 2018-07-10T18:44:17Z

src/pybind/mgr/dashboard/frontend/src/app/shared/components/modal/modal.component.scss


 .modal-header {
  @include hf;
  border-radius: 5px 5px 0 0;
 }
-
-.modal-footer {


Was this rule not being used?

I didnt see it being used anywhere, also, when I removed it as well, the modal remained unaffected.

This is actually necessary.
You will need to add the ::ng-deep, so it can be applied.
You will notice the color of the background will change.

tspmelo · 2018-07-10T18:57:13Z

src/pybind/mgr/dashboard/frontend/src/styles.scss

+@import 'defaults';
+$fa-font-path: "../node_modules/fork-awesome/fonts";
+@import "../node_modules/fork-awesome/scss/fork-awesome";
+@import 'src/app/core/navigation/navigation.module';


It's not common practice to create .scss files for modules.

If there is no parent component where you can add this rules, I would suggest you do what we did for the popover styles: create a styles/navigation.scss file and import it in every component you need it.

tspmelo · 2018-07-11T22:01:03Z

Found a few places that need fix:

Table header have the wrong style
Dropdown from the notifications have problem with icons

a2batic · 2018-07-14T05:46:37Z

@tspmelo, Corrected the styles for the following :-

a2batic · 2018-07-14T08:04:44Z

jenkins test make check

ricardoasmarques

You removed the margin-right-sm class from styles.scss, and because of that you are missing the following fix: #22759

ricardoasmarques

lgtm

tspmelo · 2018-07-24T21:54:49Z

src/pybind/mgr/dashboard/frontend/src/openattic-theme.scss

-  margin-left: 5px;
-}
-
-// awesome-bootstrap-checkbox + ForkAwesome


This rules are required for displaying the awesome-checkbox icons.
You can try it in the login page.

tspmelo · 2018-07-24T22:01:41Z

src/pybind/mgr/dashboard/frontend/src/app/ceph/dashboard/health/health.component.scss

@@ -15,3 +15,7 @@ table.ceph-chartbox {
  height: 120px;
  width: 120px;
 }
+
+::ng-deep .nav-tabs {


This should be a global rule.
We also use this in other pages.

tspmelo · 2018-07-24T22:19:04Z

...r/dashboard/frontend/src/app/ceph/cluster/osd/osd-flags-modal/osd-flags-modal.component.scss

@@ -0,0 +1,10 @@
+.osd-modal {
+  margin: 10px 0px;


You could remove both this rules and just keep the .oa-hr-small.
The others are already taken care of by bootstrap.

tspmelo · 2018-07-24T22:33:31Z

src/pybind/mgr/dashboard/frontend/src/app/shared/components/modal/modal.component.scss


 .modal-header {
  @include hf;
  border-radius: 5px 5px 0 0;
 }
-
-.modal-footer {


This is actually necessary.
You will need to add the ::ng-deep, so it can be applied.
You will notice the color of the background will change.

tspmelo · 2018-07-24T23:50:56Z

You are missing the <> before and after your email in the commit message.

tspmelo · 2018-07-25T13:04:23Z

src/pybind/mgr/dashboard/frontend/src/styles.scss

+  font-weight: normal;
+  font-style: normal;
+}
+/*For Opera browser*/


It seems Opera is no longer a problem. I'm guessing since they are now using the same render as Chrome.
This can be replaced by:

/* For awesome-bootstrap-checkbox */ .checkbox input[type='checkbox']:checked + label:after { font-family: $font_family_1; } /*******/

Also, please don't use tabs, we use 2 spaces for indentation.

Assign variables to colors present in css files Changed navigation icons padding because the notification dropdown was going out of window. Fixes: http://tracker.ceph.com/issues/24575 Signed-off-by: Kanika Murarka <kmurarka@redhat.com>

a2batic added cleanup dashboard labels Jun 25, 2018

a2batic requested review from tspmelo, votdev, ricardoasmarques and epuertat June 25, 2018 20:41

tspmelo suggested changes Jun 25, 2018

View reviewed changes

votdev requested changes Jun 27, 2018

View reviewed changes

a2batic force-pushed the scss branch 2 times, most recently from 5901e9f to 2f41a4a Compare July 5, 2018 07:28

a2batic changed the title ~~[WIP] mgr/dashboard: Color variables for color codes~~ mgr/dashboard: Color variables for color codes Jul 5, 2018

a2batic added DNM and removed DNM labels Jul 5, 2018

a2batic force-pushed the scss branch from 2f41a4a to d3be20e Compare July 5, 2018 07:32

epuertat reviewed Jul 5, 2018

View reviewed changes

epuertat requested changes Jul 5, 2018

View reviewed changes

a2batic added the DNM label Jul 5, 2018

a2batic force-pushed the scss branch 2 times, most recently from 757e309 to 480273b Compare July 10, 2018 06:44

a2batic removed the DNM label Jul 10, 2018

a2batic force-pushed the scss branch from 480273b to 5fbc149 Compare July 10, 2018 07:56

epuertat approved these changes Jul 10, 2018

View reviewed changes

tspmelo suggested changes Jul 10, 2018

View reviewed changes

a2batic force-pushed the scss branch 3 times, most recently from bb26f1c to 3b8e324 Compare July 11, 2018 17:20

a2batic force-pushed the scss branch from 3b8e324 to d44d74a Compare July 11, 2018 17:24

a2batic force-pushed the scss branch from d44d74a to 440d1a6 Compare July 14, 2018 05:41

a2batic force-pushed the scss branch 7 times, most recently from 4abbfa3 to 208f88d Compare July 21, 2018 01:04

ricardoasmarques suggested changes Jul 23, 2018

View reviewed changes

a2batic force-pushed the scss branch from 208f88d to ba44148 Compare July 23, 2018 18:46

ricardoasmarques approved these changes Jul 24, 2018

View reviewed changes

tspmelo suggested changes Jul 24, 2018

View reviewed changes

tspmelo mentioned this pull request Jul 25, 2018

mgr/dashboard: Improve prettier scripts and documentation #22994

Merged

a2batic force-pushed the scss branch from ba44148 to 9a4cd60 Compare July 25, 2018 11:49

tspmelo suggested changes Jul 25, 2018

View reviewed changes

mgr/dashboard: Color variables for color codes

7da6a25

Assign variables to colors present in css files Changed navigation icons padding because the notification dropdown was going out of window. Fixes: http://tracker.ceph.com/issues/24575 Signed-off-by: Kanika Murarka <kmurarka@redhat.com>

a2batic force-pushed the scss branch from 9a4cd60 to 7da6a25 Compare July 25, 2018 13:22

votdev approved these changes Jul 25, 2018

View reviewed changes

tspmelo approved these changes Jul 25, 2018

View reviewed changes

LenzGr merged commit f0f8a0a into ceph:master Jul 25, 2018

		@@ -62,26 +64,25 @@ export class HealthComponent implements OnInit, OnDestroy {

		preparePoolUsage(chart, data) {
		const colors = [

		@@ -0,0 +1,4 @@
		form .input-group-addon {
		color: #a2a2a2 !important;

mgr/dashboard: Color variables for color codes #22695

mgr/dashboard: Color variables for color codes #22695

Conversation

a2batic commented Jun 25, 2018 • edited

tspmelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a2batic commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callithea commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epuertat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a2batic commented Jul 10, 2018

epuertat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tspmelo commented Jul 11, 2018

a2batic commented Jul 14, 2018

a2batic commented Jul 14, 2018

ricardoasmarques left a comment

Choose a reason for hiding this comment

ricardoasmarques left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tspmelo Jul 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tspmelo commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a2batic commented Jun 25, 2018 •

edited

tspmelo Jul 24, 2018 •

edited