Design rehaul #308

remstos · 2017-07-03T18:09:00Z

Cherry-picking all design/ui work

sebgoa · 2017-07-04T09:03:50Z

let's try to get @Angelmmiguel to review.

Angelmmiguel

Thank you very much for the general refactoring. I added some comments related to the CSS and the structure of the new components.

Can you attach some screenshots with the result of the changes? It would be nice to have that information associated to the PR.

Also, I saw you changed the warn color from deep-orange to red. What's the motivation to change the main palette of the project?

Thanks!

Angelmmiguel · 2017-07-04T10:07:14Z

src/ui/src/app/chart-details/chart-details.component.html

-        <app-chart-details-info [chart]=chart [currentVersion]=currentVersion></app-chart-details-info>
-      </aside>
+      <div class="chart-details__header">
+        <div class="chart-details__header__background" [style.background-color]="chartColor"></div>


I like this concept but we don't control the color of icons. If the "main" color of an icon is to bright or dark, the readability of the content will be affected. Also, if the color is too saturated, the general design will look weird for that page.

Angelmmiguel · 2017-07-04T10:10:30Z

src/ui/src/app/chart-details/chart-details.component.scss

+    &__background {
+      width: 100%;
+      height: 200px;
+      background: md-color($monocular-app-primary);


This color will be overridden by the Icon's color when it's calculated. This will cause a flashing effect in the UI.

Angelmmiguel · 2017-07-04T10:11:09Z

src/ui/src/app/chart-details/chart-details.component.scss

    }
+  }
+  &__content {


This selector is duplicated on this file.

Angelmmiguel · 2017-07-04T10:11:36Z

src/ui/src/app/chart-details/chart-details.component.scss

+    display: flex;
+    flex-direction: column-reverse;
+    padding: 0 1em;
+    padding-bottom: 1em;


Merge this value in the previous CSS property

Angelmmiguel · 2017-07-04T10:12:27Z

src/ui/src/app/chart-details/chart-details.component.scss

+    width: 100%;
+    margin-top: -80px;
+    margin-bottom: 40px;
+    &__background {


For readability of the CSS code, can you add an space before every selector? It's helpful to follow the hierarchy in the files :)

Angelmmiguel · 2017-07-04T10:33:33Z

src/ui/src/app/deployments/deployments.component.ts

+    return list;
+  }
+
+  filterDeployments() {


I think we can create a common filter component that generate the select elements and trigger an event when some of them change.

Angelmmiguel · 2017-07-04T10:34:46Z

src/ui/src/app/header-bar/header-bar.component.scss

+    transition: background 0.2s ease;
+    padding: 0.3em 0.5em;
+    svg {
+      fill: rgba(255, 255, 255, 0.6);


Is this necessary? We can update the SVG file to use the color defined in color property instead of force the color in the fill property.

Angelmmiguel · 2017-07-04T10:35:23Z

src/ui/src/app/header-bar/header-bar.component.scss

-
-    .md-input-wrapper {
-      margin: .4em 0;
+    background: rgba(255, 255, 255, 0.125);


I'd rather have a base color and change the alpha using SASS methods

Angelmmiguel · 2017-07-04T10:36:08Z

src/ui/src/assets/icons/search.svg

@@ -0,0 +1,8 @@
+<svg height="32px" version="1.1" viewBox="0 0 32 32" width="32px" xmlns="http://www.w3.org/2000/svg" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns" xmlns:xlink="http://www.w3.org/1999/xlink">


Use svgo to reduce the size of the SVG file

Angelmmiguel · 2017-07-04T10:36:51Z

src/ui/src/index.html

-    (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
-    m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
-    })(window,document,'script','https://www.google-analytics.com/analytics.js','ga');
+  (function(i, s, o, g, r, a, m) {


Why did you uncompress the Google Analytics method and change the indentation of the script?

Angelmmiguel · 2017-07-04T10:56:23Z

Related to my comments about taking the color of the icons, this is the problem I see:

Also, what's the motivation to fix the size of the UI to 1000px? I think it's too narrow for some of the pages like details. At the beginning of the project we decided to use the 100% of the screen.

Related to the cards, the icon is very relevant to identify a product. Now, the main element in the card is the color of the icon and they are smaller. Also, sometimes that color doesn't match with the real brand color.

About the Home page, the cards have more margin on the right of the page:

remstos · 2017-07-04T16:50:37Z

Thank you @Angelmmiguel , this is probably the best review I've ever seen. I definitely moved without being aware of some guidelines you use, I'll fix this first and then I reply to each question.

Angelmmiguel · 2017-07-05T13:35:09Z

Great @kemcake! Btw, the purpose of some requested changes is to be consistent with the rest of the project. If you have any suggestion about how to improve the current approach, please feel free to propose them so we can do a review. Thanks!

remstos · 2017-07-07T18:31:34Z

So the remaining questions are:

Should we use that color theming depending on the logo, looks nice most of the time but I agree it has some risks. The idea was to put more color and add some life to a chart. There's some checks on the color script to make sure we don't select too bright/dark color, but this is maybe not enough.
max width layout is a common thing (github...), 100% looks really strange on a wide screen and is usually bad for readable stuff like the readme. Once again this is a suggestion, I moved the value to theme.scss so it's easily editable.

codecov-io · 2017-07-07T21:31:24Z

Codecov Report

Merging #308 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   88.96%   88.99%   +0.03%     
==========================================
  Files          15       15              
  Lines         616      618       +2     
==========================================
+ Hits          548      550       +2     
  Misses         45       45              
  Partials       23       23

Impacted Files	Coverage Δ
src/api/mocks/client.go	`100% <ø> (ø)`	⬆️
src/api/handlers/releases/releases.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ab28c6...6176220. Read the comment docs.

sebgoa · 2017-07-08T16:24:40Z

personally I am fine with the fixed width. I agree that on big screens it stretches things to a very weird point.

For the colors, I don't really care. The main problem is that we do not have logos for all charts.

@Angelmmiguel can you do a final review so that we can move forward.

This can always be revisited.

Angelmmiguel · 2017-07-10T07:54:51Z

@kemcake great job! Thanks for the improvements and refactoring. About the remaining items:

Should we use that color theming depending on the logo, looks nice most of the time but I agree it has some risks. The idea was to put more color and add some life to a chart. There's some checks on the color script to make sure we don't select too bright/dark color, but this is maybe not enough.

I'd remove this feature. I prefer to have a controlled UI since the base color of the logos may not fit with the current design. Also, I think it's better to continue with the initial design of the cards. We did a research about the cards and decided to keep logos as the main and more important element.

max width layout is a common thing (github...), 100% looks really strange on a wide screen and is usually bad for readable stuff like the readme. Once again this is a suggestion, I moved the value to theme.scss so it's easily editable.

That's solution works for me! Also, can you increase a bit that fixed width value? I believe that a value between 1150-1250px will work in most use cases.

Thanks!

Angelmmiguel

Review my last comments before merge it :)

prydonius · 2017-07-12T15:49:49Z

This looks great to me, only thing I noticed was the arrows in the Install section:

Oddly, if you click on "Helm CLI" it goes back to normal:

I don't mind if you prefer to merge this and fix this in a separate PR though.

sebgoa · 2017-07-17T09:34:44Z

@kemcake can you fix the conflicts, rebase and then we will merge.

prydonius · 2017-07-18T12:59:08Z

This worked before but I'm now unable to compile this again:

ui_1   | Time: 62932ms
ui_1   | chunk    {0} main.bundle.js, main.bundle.map (main) 591 kB {2} [initial] [rendered]
ui_1   | chunk    {1} styles.bundle.css, styles.bundle.map, styles.bundle.map (styles) 69.5 kB {3} [initial] [rendered]
ui_1   | chunk    {2} vendor.bundle.js, vendor.bundle.map (vendor) 4.75 MB [initial] [rendered]
ui_1   | chunk    {3} inline.bundle.js, inline.bundle.map (inline) 0 bytes [entry] [rendered]
ui_1   |
ui_1   | ERROR in /app/src/app/chart-details/chart-details-readme/chart-details-readme.component.ts (2,22): Cannot find module 'marked'.
ui_1   |
ui_1   | ERROR in /app/src/app/chart-details/chart-details-info/chart-details-info.component.ts (6,21): Cannot find module 'url-join'.
ui_1   | webpack: Failed to compile.

Really not sure why these errors keep coming up...

prydonius · 2017-07-19T13:05:20Z

@kemkake I managed to get rid of the error I was getting above by installing the typings (https://www.reddit.com/r/Angular2/comments/59b2mc/import_vs_require_why_one_work_and_the_other/) and changing the import line.

npm install --save-dev @types/marked
npm install --save-dev @types/url-join

diff --git a/src/ui/package.json b/src/ui/package.json
index 5e5fe7a..c7e28ac 100644
--- a/src/ui/package.json
+++ b/src/ui/package.json
@@ -46,6 +46,8 @@
   },
   "devDependencies": {
     "@types/jasmine": "^2.2.30",
+    "@types/marked": "0.0.28",
+    "@types/url-join": "^0.8.2",
     "angular-cli": "1.0.0-beta.24",
     "bootstrap-sass": "^3.3.7",
     "codelyzer": "~0.0.26",
@@ -57,6 +59,7 @@
     "karma-cli": "^1.0.1",
     "karma-jasmine": "^1.0.2",
     "karma-remap-istanbul": "^0.2.1",
+    "mappy-breakpoints": "^0.2.3",
     "node-sass": "^3.9.3",
     "protractor": "4.0.5",
     "resolve-url-loader": "^1.6.0",
@@ -66,7 +69,6 @@
     "tslint": "3.13.0",
     "typescript": "^2.0.0",
     "url-loader": "^0.5.7",
-    "mappy-breakpoints": "^0.2.3",
     "webpack": "2.2.0-rc.3"
   }
 }
diff --git a/src/ui/src/app/chart-details/chart-details-info/chart-details-info.component.ts b/src/ui/src/app/chart-details/chart-details-info/chart-details-info.component.ts
index 115453a..51445c1 100644
--- a/src/ui/src/app/chart-details/chart-details-info/chart-details-info.component.ts
+++ b/src/ui/src/app/chart-details/chart-details-info/chart-details-info.component.ts
@@ -3,7 +3,7 @@ import { ChartsService } from '../../shared/services/charts.service';
 import { Chart } from '../../shared/models/chart';
 import { Maintainer } from '../../shared/models/maintainer';
 import { ChartVersion } from '../../shared/models/chart-version';
-import urljoin from 'url-join';
+import * as urljoin from 'url-join';

 @Component({
   selector: 'app-chart-details-info',
diff --git a/src/ui/src/app/chart-details/chart-details-readme/chart-details-readme.component.ts b/src/ui/src/app/chart-details/chart-details-readme/chart-details-readme.component.ts
index 840b0c8..4f3022f 100644
--- a/src/ui/src/app/chart-details/chart-details-readme/chart-details-readme.component.ts
+++ b/src/ui/src/app/chart-details/chart-details-readme/chart-details-readme.component.ts
@@ -1,5 +1,5 @@
 import { Component, Input, OnChanges, SimpleChanges } from '@angular/core';
-import markdown from 'marked';
+import * as markdown from 'marked';
 import { Chart } from '../../shared/models/chart';
 import { ChartsService } from '../../shared/services/charts.service';
 import { ChartVersion } from '../../shared/models/chart-version';

prydonius · 2017-07-19T13:06:16Z

If the above works in your environment too, can we update to it and then land? We can revisit these issues when we move start from the new angular project with yarn.

remstos added 27 commits July 3, 2017 10:52

Reorganize deployment list

324b6f6

Add ChartIcon to deployment

fb97a75

Add filters + Search input

8145424

Fix link + default namespace

addbeb4

Deployment details layout update

98e23f4

ChartIcon in release extended

d4a00db

Add chart icon color theming

f193bc1

Fix list alignments

583f53b

Prevent chart icon not showing up

4a01a11

Update Swagger spec for chartIcon

1f0ae34

Deployment subtitle style clean

0bc8332

Cleanup headerbar

8827fe1

Add deployment to small menu

e752a5b

Improve small navbar style/animation

28f0726

Colorful card style chart-item

c0e01fc

Reorganize charts list

58c1b86

Chart Details profile style

89c9452

Charts list overall ui fixes

2889774

Allow search in repo

82963f6

Use custom module for color picking

ef5aa82

Fix too long chart/deployment name helm#16

b6366f5

Add search icon on charts/deployments search field

c0b1306

Fix UI Tests

c9b840e

Fix chart list alignment + no charts result

ac1455f

Improve search bars

73c0a36

Remove chart-search route

bac893d

Fix empty charts list state

fe27232

remstos changed the title ~~Design rehaul [WIP]~~ Design rehaul Jul 3, 2017

Angelmmiguel suggested changes Jul 4, 2017

View reviewed changes

remstos added 2 commits July 4, 2017 10:33

Clean sass selector alignment

87183c6

Css variable cleanup + Fix most review comments

dfaba86

remstos added 4 commits July 5, 2017 10:35

Use colors from theme.scss everywhere

c3f5798

Optimize search svg

d5597f1

Refactor List Item

0d9172a

Refactor list filters for chart & deployments

13513ed

remstos added 2 commits July 7, 2017 12:42

Relayout header-bar + maxwidth layout on main page

1a0590e

Fix test and compile script

deda266

Angelmmiguel approved these changes Jul 10, 2017

View reviewed changes

remstos added 2 commits July 10, 2017 10:02

Back to previous chart layout + large max layout width

5a50227

Add xlarge breakpoint

83526ad

Merge branch 'master' into design-rehaul

eec4c0a

remstos added 3 commits July 19, 2017 09:16

Fix url-join and marked imports

a41dbbc

Update swagger spec

cd9a637

Merge branch 'master' into design-rehaul

6176220

prydonius merged commit 078e531 into helm:master Jul 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Design rehaul #308

Design rehaul #308

remstos commented Jul 3, 2017 •

edited

Loading

sebgoa commented Jul 4, 2017

Angelmmiguel left a comment

Angelmmiguel Jul 4, 2017

Angelmmiguel Jul 4, 2017

Angelmmiguel Jul 4, 2017

Angelmmiguel Jul 4, 2017

Angelmmiguel Jul 4, 2017

Angelmmiguel Jul 4, 2017

Angelmmiguel Jul 4, 2017

Angelmmiguel Jul 4, 2017

Angelmmiguel Jul 4, 2017

Angelmmiguel Jul 4, 2017

Angelmmiguel commented Jul 4, 2017

remstos commented Jul 4, 2017

Angelmmiguel commented Jul 5, 2017

remstos commented Jul 7, 2017

codecov-io commented Jul 7, 2017 •

edited

Loading

sebgoa commented Jul 8, 2017

Angelmmiguel commented Jul 10, 2017

Angelmmiguel left a comment

prydonius commented Jul 12, 2017

sebgoa commented Jul 17, 2017

prydonius commented Jul 18, 2017

prydonius commented Jul 19, 2017

prydonius commented Jul 19, 2017

		@@ -0,0 +1,8 @@
		<svg height="32px" version="1.1" viewBox="0 0 32 32" width="32px" xmlns="http://www.w3.org/2000/svg" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns" xmlns:xlink="http://www.w3.org/1999/xlink">

Design rehaul #308

Design rehaul #308

Conversation

remstos commented Jul 3, 2017 • edited Loading

sebgoa commented Jul 4, 2017

Angelmmiguel 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

Angelmmiguel commented Jul 4, 2017

remstos commented Jul 4, 2017

Angelmmiguel commented Jul 5, 2017

remstos commented Jul 7, 2017

codecov-io commented Jul 7, 2017 • edited Loading

Codecov Report

sebgoa commented Jul 8, 2017

Angelmmiguel commented Jul 10, 2017

Angelmmiguel left a comment

Choose a reason for hiding this comment

prydonius commented Jul 12, 2017

sebgoa commented Jul 17, 2017

prydonius commented Jul 18, 2017

prydonius commented Jul 19, 2017

prydonius commented Jul 19, 2017

remstos commented Jul 3, 2017 •

edited

Loading

codecov-io commented Jul 7, 2017 •

edited

Loading