Bug 1575499 - Part 3: UI fixes and CSS refactor r=Ola
authorBelén Albeza <balbeza@mozilla.com>
Mon, 26 Aug 2019 14:42:38 +0000
changeset 489874 63c3e7548bc0dcbd1deea6798eb8a5742e3db820
parent 489873 1202ceb560284c2b0b7a3b54593b68ad6fff9e1c
child 489875 a5d956f10856d3e3639a9ecc02329f17eefcc92f
push id36491
push usermalexandru@mozilla.com
push dateMon, 26 Aug 2019 22:30:36 +0000
treeherdermozilla-central@5c7635de0cb6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersOla
bugs1575499, 1575872
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1575499 - Part 3: UI fixes and CSS refactor r=Ola > IMPORTANT: this depends on https://phabricator.services.mozilla.com/D43050 to be on central. If it has not been merged already, run `arc patch D43050` before downloading this stack. - Put style rules into their own components stylesheet. - Used theme-related colors instead of the same ones for both dark and light themes. - Made selectors to be class-specific (so they have low specificity) - Removed some unused CSS in `base.css` - Fix extra blank space when a section of the manifest is empty (made the hardcoded icons array empty so this can be seen) NOTE: The CSS/markup for the warnings will be handled in https://bugzilla.mozilla.org/show_bug.cgi?id=1575872 Differential Revision: https://phabricator.services.mozilla.com/D43231
devtools/client/application/application.css
devtools/client/application/src/base.css
devtools/client/application/src/components/manifest/Manifest.css
devtools/client/application/src/components/manifest/Manifest.js
devtools/client/application/src/components/manifest/ManifestItem.css
devtools/client/application/src/components/manifest/ManifestItem.js
devtools/client/application/src/components/manifest/ManifestItemWarning.css
devtools/client/application/src/components/manifest/ManifestItemWarning.js
devtools/client/application/src/components/manifest/ManifestPage.js
devtools/client/application/src/components/manifest/ManifestSection.css
devtools/client/application/src/components/manifest/ManifestSection.js
devtools/client/application/src/components/manifest/moz.build
devtools/client/application/src/components/routing/PageSwitcher.css
devtools/client/application/src/components/service-workers/Worker.css
devtools/client/application/src/components/service-workers/WorkerList.js
devtools/client/application/test/components/manifest/__snapshots__/components_application_panel-ManifestPage.test.js.snap
devtools/client/application/test/components/service-workers/__snapshots__/components_application_panel-WorkerList.test.js.snap
--- a/devtools/client/application/application.css
+++ b/devtools/client/application/application.css
@@ -6,17 +6,19 @@
 * Global styles
 */
 @import "resource://devtools/client/application/src/base.css";
 
 /*
 * Components
 */
 @import "resource://devtools/client/application/src/components/App.css";
-@import "resource://devtools/client/application/src/components/manifest/Manifest.css";
+@import "resource://devtools/client/application/src/components/manifest/ManifestItem.css";
+@import "resource://devtools/client/application/src/components/manifest/ManifestItemWarning.css";
+@import "resource://devtools/client/application/src/components/manifest/ManifestSection.css";
 @import "resource://devtools/client/application/src/components/routing/PageSwitcher.css";
 @import "resource://devtools/client/application/src/components/service-workers/Worker.css";
 @import "resource://devtools/client/application/src/components/service-workers/WorkerList.css";
 @import "resource://devtools/client/application/src/components/service-workers/WorkerListEmpty.css";
 @import "resource://devtools/client/application/src/components/service-workers/WorkersPage.css";
 @import "resource://devtools/client/application/src/components/ui/UIButton.css";
 
 html,
--- a/devtools/client/application/src/base.css
+++ b/devtools/client/application/src/base.css
@@ -20,24 +20,23 @@
 
   /* Global styles */
   --base-font-size: var(--body-10-font-size);
   --base-font-weight: var(--body-10-font-weight);
   --base-line-height: 1.8;
   --list-line-height: 1.25;
 
  /* Global colours */
-  --bg-color: var(--grey-10);
-  --text-color: var(--grey-90);
+  --separator-color: var(--theme-text-color-alt);
+
+  /* extra, raw colors */
+  --blue-50-a30: rgba(10, 132, 255, 0.3);
 
   /* Global layout vars */
   --base-unit: 4px;
-
-  /* extra, raw colors */
-  --blue-50-a30: rgba(10, 132, 255, 0.3);
 }
 
 /*
 * Reset some tags
 */
 
 * {
   box-sizing: border-box;
@@ -54,18 +53,11 @@ ul {
 }
 
 a {
   color: var(--theme-highlight-blue);
   text-decoration: none;
   cursor: pointer;
 }
 
-h1,
-.application--title {
-  font-size: var(--title-30-font-size);
-  font-weight: var(--title-30-font-weight);
-  line-height: var(--base-line-height);
-}
-
 p {
   margin: 0;
 }
--- a/devtools/client/application/src/components/manifest/Manifest.js
+++ b/devtools/client/application/src/components/manifest/Manifest.js
@@ -18,17 +18,17 @@ const FluentReact = require("devtools/cl
 const Localized = createFactory(FluentReact.Localized);
 const { l10n } = require("../../modules/l10n");
 
 const ManifestItem = createFactory(require("./ManifestItem"));
 const ManifestItemWarning = createFactory(require("./ManifestItemWarning"));
 const ManifestSection = createFactory(require("./ManifestSection"));
 
 /**
- * Displays a canonical manifest, splitted in different sections
+ * A canonical manifest, splitted in different sections
  */
 class Manifest extends PureComponent {
   static get propTypes() {
     return {
       icons: PropTypes.array.isRequired,
       identity: PropTypes.array.isRequired,
       presentation: PropTypes.array.isRequired,
       warnings: PropTypes.array.isRequired,
new file mode 100644
--- /dev/null
+++ b/devtools/client/application/src/components/manifest/ManifestItem.css
@@ -0,0 +1,18 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+.manifest-item {
+  vertical-align: baseline;
+}
+
+.manifest-item__label {
+  color: var(--theme-text-color-alt);
+  font-weight: var(--title-30-font-weight);
+  width: calc(var(--base-unit) * 28);
+  text-align: right;
+}
+
+.manifest-item__value {
+  word-break: break-all;
+}
--- a/devtools/client/application/src/components/manifest/ManifestItem.js
+++ b/devtools/client/application/src/components/manifest/ManifestItem.js
@@ -22,24 +22,24 @@ class ManifestItem extends PureComponent
       children: PropTypes.node,
     };
   }
 
   render() {
     const { children, label } = this.props;
     return tr(
       {
-        className: "manifest__row",
+        className: "manifest-item",
       },
       th(
         {
-          className: "manifest__col-label",
+          className: "manifest-item__label",
           scope: "row",
         },
         label
       ),
-      td({ className: "manifest__col-value" }, children)
+      td({ className: "manifest-item__value" }, children)
     );
   }
 }
 
 // Exports
 module.exports = ManifestItem;
new file mode 100644
--- /dev/null
+++ b/devtools/client/application/src/components/manifest/ManifestItemWarning.css
@@ -0,0 +1,8 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+.manifest-warning {
+  vertical-align: top;
+}
+
--- a/devtools/client/application/src/components/manifest/ManifestItemWarning.js
+++ b/devtools/client/application/src/components/manifest/ManifestItemWarning.js
@@ -9,35 +9,43 @@ const { PureComponent } = require("devto
 const {
   tr,
   td,
   th,
   img,
 } = require("devtools/client/shared/vendor/react-dom-factories");
 
 /**
- * This component
+ * A Manifest warning validation message
  */
 class ManifestItemWarning extends PureComponent {
+  // TODO: this probably should not be a table, but a list. It might also make
+  // more sense to rename to ManifestIssue. Address in:
+  // https://bugzilla.mozilla.org/show_bug.cgi?id=1575872
+
   static get propTypes() {
+    // TODO: pass multiple props instead of just a single object
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1575872
     return {
       warning: PropTypes.object.isRequired,
     };
   }
+
   render() {
     const { warning } = this.props;
-
     return tr(
-      { className: "manifest__row manifest__row-error" },
+      { className: "manifest-warning" },
       th(
-        { className: "manifest__col-label", scope: "row" },
+        { scope: "row" },
         img({
+          // TODO: Add a localized string in
+          // https://bugzilla.mozilla.org/show_bug.cgi?id=1575872
+          alt: "Warning icon",
           src: "chrome://global/skin/icons/warning.svg",
-          alt: "Warning icon",
         })
       ),
-      td({ className: "manifest__col-value" }, warning.warn)
+      td({}, warning.warn)
     );
   }
 }
 
 // Exports
 module.exports = ManifestItemWarning;
--- a/devtools/client/application/src/components/manifest/ManifestPage.js
+++ b/devtools/client/application/src/components/manifest/ManifestPage.js
@@ -23,33 +23,17 @@ class ManifestPage extends PureComponent
     const data = {
       warnings: [
         { warn: "Icons item at index 0 is invalid." },
         {
           warn:
             "Icons item at index 2 is invalid. Icons item at index 2 is invalid. Icons item at index 2 is invalid. Icons item at index 2 is invalid.",
         },
       ],
-      icons: [
-        {
-          key: "16x16",
-          value:
-            "https://design.firefox.com/icons/icons/desktop/default-browser-16.svg",
-        },
-        {
-          key: "32x32",
-          value:
-            "https://design.firefox.com/icons/icons/desktop/default-browser-16.svg",
-        },
-        {
-          key: "64x64",
-          value:
-            "https://design.firefox.com/icons/icons/desktop/default-browser-16.svg",
-        },
-      ],
+      icons: [],
       identity: [
         {
           key: "name",
           value:
             "Name is a verrry long name and the name is longer tha you thinnk because it is loooooooooooooooooooooooooooooooooooooooooooooooong",
         },
         {
           key: "short_name",
rename from devtools/client/application/src/components/manifest/Manifest.css
rename to devtools/client/application/src/components/manifest/ManifestSection.css
--- a/devtools/client/application/src/components/manifest/Manifest.css
+++ b/devtools/client/application/src/components/manifest/ManifestSection.css
@@ -1,41 +1,24 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-.manifest {
-  padding: calc(var(--base-unit) * 2) calc(var(--base-unit) * 4);
-  border-bottom: 1px solid var(--grey-20);
-  display: table;
+.manifest-section {
+  padding-block: calc(var(--base-unit) * 2);
   width: 100%;
+  border-spacing: calc(var(--base-unit) * 2) 0;
 }
 
-.manifest:last-child {
-  border-bottom: 0;
-}
-
-.manifest__title {
-	font-size: var(--title-10-font-size);
-  font-weight: var(--title-20-font-weight);
-  text-align: left;
-  padding: calc(var(--base-unit) * 2) 0;
+.manifest-section--empty {
+  padding-block-end: 0;
 }
 
-.manifest__row {
-  vertical-align: top;
+.manifest-section:not(:last-child) {
+  border-bottom: 1px solid var(--separator-color);
 }
 
-.manifest__col-label {
-	color: var(--grey-50);
-  font-weight: var(--title-30-font-weight);
-  width: calc(var(--base-unit) * 28);
-  text-align: right;
+.manifest-section__title {
+  font-size: var(--title-10-font-size);
+  font-weight: var(--title-20-font-weight);
+  text-align: left;
+  padding-block-start: calc(var(--base-unit) * 2);
 }
-
-.manifest__col-value {
-  word-break: break-all;
-}
-
-.manifest__row-error .manifest__col-label {
-  width: calc(var(--base-unit) * 6);
-  text-align: left;
-}
--- a/devtools/client/application/src/components/manifest/ManifestSection.js
+++ b/devtools/client/application/src/components/manifest/ManifestSection.js
@@ -8,31 +8,36 @@ const PropTypes = require("devtools/clie
 const { PureComponent } = require("devtools/client/shared/vendor/react");
 const {
   caption,
   table,
   tbody,
 } = require("devtools/client/shared/vendor/react-dom-factories");
 
 /**
- * Displays a section of a manifest in the form of a captioned table.
+ * A section of a manifest in the form of a captioned table.
  */
 class ManifestSection extends PureComponent {
   static get propTypes() {
     return {
       children: PropTypes.node,
       title: PropTypes.string.isRequired,
     };
   }
 
   render() {
     const { children, title } = this.props;
+    const isEmpty = !children || children.length === 0;
 
     return table(
-      { className: "manifest" },
-      caption({ className: "manifest__title" }, title),
+      {
+        className: `manifest-section ${
+          isEmpty ? "manifest-section--empty" : ""
+        }`,
+      },
+      caption({ className: "manifest-section__title" }, title),
       tbody({}, children)
     );
   }
 }
 
 // Exports
 module.exports = ManifestSection;
--- a/devtools/client/application/src/components/manifest/moz.build
+++ b/devtools/client/application/src/components/manifest/moz.build
@@ -1,14 +1,16 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 DevToolsModules(
-    'Manifest.css',
     'Manifest.js',
     'ManifestEmpty.js',
+    'ManifestItem.css',
     'ManifestItem.js',
+	'ManifestItemWarning.css',
 	'ManifestItemWarning.js',
     'ManifestLoader.js',
     'ManifestPage.js',
+    'ManifestSection.css',
     'ManifestSection.js',
 )
--- a/devtools/client/application/src/components/routing/PageSwitcher.css
+++ b/devtools/client/application/src/components/routing/PageSwitcher.css
@@ -6,17 +6,16 @@
 /*
  * Page container for worker + manifest views
  */
 
 .app-page {
   height: 100vh;
   padding: 0 2rem;
   display: grid;
-  grid-template-rows: 1fr auto;
   -moz-user-select: none;
 }
 
 .app-page--empty {
   align-items: center;
   justify-content: center;
   font-size: var(--body-10-font-size);
   color: var(--theme-toolbar-color);
--- a/devtools/client/application/src/components/service-workers/Worker.css
+++ b/devtools/client/application/src/components/service-workers/Worker.css
@@ -26,17 +26,17 @@
   font-size: var(--body-10-font-size);
 }
 
 .worker:first-child {
   padding-top: 0;
 }
 
 .worker:not(:last-child) {
-  border-bottom: 1px solid var(--theme-text-color-alt);
+  border-bottom: 1px solid var(--separator-color);
 }
 
 .worker__header {
   grid-column: 1/3;
   display: grid;
   grid-template-columns: 1fr auto;
   grid-column-gap: 2rem;
   align-items: center;
--- a/devtools/client/application/src/components/service-workers/WorkerList.js
+++ b/devtools/client/application/src/components/service-workers/WorkerList.js
@@ -39,17 +39,17 @@ class WorkerList extends PureComponent {
     const { canDebugWorkers, workers } = this.props;
 
     return [
       article(
         { className: "workers-container", key: "workers-container" },
         Localized(
           { id: "serviceworker-list-header" },
           h1({
-            className: "application--title",
+            className: "app-page__title",
           })
         ),
         ul(
           {},
           workers.map(worker =>
             Worker({
               key: worker.id,
               isDebugEnabled: canDebugWorkers,
--- a/devtools/client/application/test/components/manifest/__snapshots__/components_application_panel-ManifestPage.test.js.snap
+++ b/devtools/client/application/test/components/manifest/__snapshots__/components_application_panel-ManifestPage.test.js.snap
@@ -1,32 +1,17 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
 exports[`ManifestPage renders the expected snapshot 1`] = `
 <section
   className="app-page "
 >
   <Connect(ManifestLoader) />
   <Manifest
-    icons={
-      Array [
-        Object {
-          "key": "16x16",
-          "value": "https://design.firefox.com/icons/icons/desktop/default-browser-16.svg",
-        },
-        Object {
-          "key": "32x32",
-          "value": "https://design.firefox.com/icons/icons/desktop/default-browser-16.svg",
-        },
-        Object {
-          "key": "64x64",
-          "value": "https://design.firefox.com/icons/icons/desktop/default-browser-16.svg",
-        },
-      ]
-    }
+    icons={Array []}
     identity={
       Array [
         Object {
           "key": "name",
           "value": "Name is a verrry long name and the name is longer tha you thinnk because it is loooooooooooooooooooooooooooooooooooooooooooooooong",
         },
         Object {
           "key": "short_name",
--- a/devtools/client/application/test/components/service-workers/__snapshots__/components_application_panel-WorkerList.test.js.snap
+++ b/devtools/client/application/test/components/service-workers/__snapshots__/components_application_panel-WorkerList.test.js.snap
@@ -5,17 +5,17 @@ Array [
   <article
     className="workers-container"
     key="workers-container"
   >
     <Localized
       id="serviceworker-list-header"
     >
       <h1
-        className="application--title"
+        className="app-page__title"
       />
     </Localized>
     <ul>
       <Worker
         isDebugEnabled={true}
         worker={
           Object {
             "active": true,
@@ -51,17 +51,17 @@ Array [
   <article
     className="workers-container"
     key="workers-container"
   >
     <Localized
       id="serviceworker-list-header"
     >
       <h1
-        className="application--title"
+        className="app-page__title"
       />
     </Localized>
     <ul>
       <Worker
         isDebugEnabled={true}
         worker={
           Object {
             "active": true,