Bug 1494543 - Part 1: Refactor sidebar items into different components. r=jdescottes,daisuke
☠☠ backed out by 63123603efb8 ☠ ☠
authorBelén Albeza <balbeza@mozilla.com>
Thu, 11 Oct 2018 11:59:26 +0000
changeset 499687 5a54f333b855138687b5b71e33c1c6144fc7fbe5
parent 499686 5f44f57c4ae734a8c9ac490c4e520c703365beae
child 499688 a871d40e8802cc8f049548ba40ccebe550a69c83
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes, daisuke
bugs1494543
milestone64.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 1494543 - Part 1: Refactor sidebar items into different components. r=jdescottes,daisuke This is a refactor of the components used in the sidebar. TL;DR: sidebar items now use the composition approach outlined here https://reactjs.org/docs/composition-vs-inheritance.html Before we had a container `Sidebar` component, which in turn had `SidebarItem` components inside. The issue was that depending on what item is inside, the information and UX displayed is different. Before this patch, we had an optional commponent, `DeviceSidebarItemAction` –which was featuring a "Connect" button, and was only rendered in the runtime sidebar items. However, we now need to display even more info, so continue to pass optional components to `SidebarItem` was tricky. What this patch does is to preserve `SidebarItem` and treat is a generic container of more specific content. This is passed via the `children` prop, which React automatically maps to the DOM content that we pass to that component (this is the same concept as slots in Web Components / Vue). `SidebarItem` now only contains the logic to select items in the sidebar and render them in `<li>` elements. Two new components, `SidebarFixedItem` (for our "static" pages) and `SidebarDeviceItem` are now the ones instancing `SidebarItem` with their specific contents. Differential Revision: https://phabricator.services.mozilla.com/D7704
devtools/client/aboutdebugging-new/aboutdebugging.css
devtools/client/aboutdebugging-new/src/components/sidebar/DeviceSidebarItemAction.css
devtools/client/aboutdebugging-new/src/components/sidebar/DeviceSidebarItemAction.js
devtools/client/aboutdebugging-new/src/components/sidebar/Sidebar.js
devtools/client/aboutdebugging-new/src/components/sidebar/SidebarFixedItem.css
devtools/client/aboutdebugging-new/src/components/sidebar/SidebarFixedItem.js
devtools/client/aboutdebugging-new/src/components/sidebar/SidebarItem.css
devtools/client/aboutdebugging-new/src/components/sidebar/SidebarItem.js
devtools/client/aboutdebugging-new/src/components/sidebar/SidebarRuntimeItem.css
devtools/client/aboutdebugging-new/src/components/sidebar/SidebarRuntimeItem.js
devtools/client/aboutdebugging-new/src/components/sidebar/moz.build
--- a/devtools/client/aboutdebugging-new/aboutdebugging.css
+++ b/devtools/client/aboutdebugging-new/aboutdebugging.css
@@ -11,17 +11,19 @@
 @import "resource://devtools/client/aboutdebugging-new/src/components/connect/NetworkLocationsList.css";
 @import "resource://devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetItem.css";
 @import "resource://devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetList.css";
 @import "resource://devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetPane.css";
 @import "resource://devtools/client/aboutdebugging-new/src/components/debugtarget/ExtensionDetail.css";
 @import "resource://devtools/client/aboutdebugging-new/src/components/debugtarget/WorkerDetail.css";
 @import "resource://devtools/client/aboutdebugging-new/src/components/sidebar/DeviceSidebarItemAction.css";
 @import "resource://devtools/client/aboutdebugging-new/src/components/sidebar/Sidebar.css";
+@import "resource://devtools/client/aboutdebugging-new/src/components/sidebar/SidebarFixedItem.css";
 @import "resource://devtools/client/aboutdebugging-new/src/components/sidebar/SidebarItem.css";
+@import "resource://devtools/client/aboutdebugging-new/src/components/sidebar/SidebarRuntimeItem.css";
 
 :root {
   /* Import css variables from common.css */
   --text-color: var(--in-content-page-color);
 
   /* */
   /* Variables with values from common.css, which are hardcoded there */
   /* */
deleted file mode 100644
--- a/devtools/client/aboutdebugging-new/src/components/sidebar/DeviceSidebarItemAction.css
+++ /dev/null
@@ -1,7 +0,0 @@
-/* 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/. */
-
-.sidebar-item__connect-button {
-  font-size: 0.8em;
-}
\ No newline at end of file
deleted file mode 100644
--- a/devtools/client/aboutdebugging-new/src/components/sidebar/DeviceSidebarItemAction.js
+++ /dev/null
@@ -1,57 +0,0 @@
-/* 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/. */
-
-"use strict";
-
-const { createFactory, PureComponent } = require("devtools/client/shared/vendor/react");
-const dom = require("devtools/client/shared/vendor/react-dom-factories");
-const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
-
-const FluentReact = require("devtools/client/shared/vendor/fluent-react");
-const Localized = createFactory(FluentReact.Localized);
-
-const Actions = require("../../actions/index");
-
-/**
- * This component displays actions for sidebar items representing a device.
- */
-class DeviceSidebarItemAction extends PureComponent {
-  static get propTypes() {
-    return {
-      connected: PropTypes.bool.isRequired,
-      dispatch: PropTypes.func.isRequired,
-      runtimeId: PropTypes.string.isRequired,
-    };
-  }
-
-  render() {
-    const { connected } = this.props;
-    if (connected) {
-      return Localized(
-        {
-          id: "about-debugging-sidebar-item-connected-label"
-        },
-        dom.span({}, "Connected")
-      );
-    }
-
-    return Localized(
-      {
-        id: "about-debugging-sidebar-item-connect-button"
-      },
-      dom.button(
-        {
-          className: "sidebar-item__connect-button",
-          onClick: () => {
-            const { dispatch, runtimeId } = this.props;
-            dispatch(Actions.connectRuntime(runtimeId));
-          }
-        },
-        "Connect"
-      )
-    );
-  }
-}
-
-module.exports = DeviceSidebarItemAction;
--- a/devtools/client/aboutdebugging-new/src/components/sidebar/Sidebar.js
+++ b/devtools/client/aboutdebugging-new/src/components/sidebar/Sidebar.js
@@ -8,18 +8,18 @@ const { createFactory, PureComponent } =
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
 const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
 
 const FluentReact = require("devtools/client/shared/vendor/fluent-react");
 const Localized = createFactory(FluentReact.Localized);
 
 const { PAGES, RUNTIMES } = require("../../constants");
 
-const DeviceSidebarItemAction = createFactory(require("./DeviceSidebarItemAction"));
-const SidebarItem = createFactory(require("./SidebarItem"));
+const SidebarFixedItem = createFactory(require("./SidebarFixedItem"));
+const SidebarRuntimeItem = createFactory(require("./SidebarRuntimeItem"));
 const FIREFOX_ICON = "chrome://devtools/skin/images/aboutdebugging-firefox-logo.svg";
 const CONNECT_ICON = "chrome://devtools/skin/images/aboutdebugging-connect-icon.svg";
 const GLOBE_ICON = "chrome://devtools/skin/images/aboutdebugging-globe-icon.svg";
 const USB_ICON = "chrome://devtools/skin/images/aboutdebugging-connect-icon.svg";
 
 class Sidebar extends PureComponent {
   static get propTypes() {
     return {
@@ -53,66 +53,57 @@ class Sidebar extends PureComponent {
 
   renderSidebarItems(icon, runtimes) {
     const { dispatch, selectedPage } = this.props;
 
     return runtimes.map(runtime => {
       const pageId = runtime.type + "-" + runtime.id;
       const runtimeHasConnection = !!runtime.connection;
 
-      const connectComponent = DeviceSidebarItemAction({
-        connected: runtimeHasConnection,
-        dispatch,
-        runtimeId: runtime.id,
-      });
-
-      return SidebarItem({
-        connectComponent,
+      return SidebarRuntimeItem({
         id: pageId,
         dispatch,
         icon,
+        isConnected: runtimeHasConnection,
         isSelected: selectedPage === pageId,
         key: pageId,
         name: runtime.name,
         runtimeId: runtime.id,
-        selectable: runtimeHasConnection,
       });
     });
   }
 
   render() {
     const { dispatch, selectedPage } = this.props;
 
     return dom.aside(
       {
         className: `sidebar ${this.props.className || ""}`,
       },
       dom.ul(
         {},
         Localized(
           { id: "about-debugging-sidebar-this-firefox", attrs: { name: true } },
-          SidebarItem({
+          SidebarFixedItem({
             id: PAGES.THIS_FIREFOX,
             dispatch,
             icon: FIREFOX_ICON,
             isSelected: PAGES.THIS_FIREFOX === selectedPage,
             name: "This Firefox",
             runtimeId: RUNTIMES.THIS_FIREFOX,
-            selectable: true,
           })
         ),
         Localized(
           { id: "about-debugging-sidebar-connect", attrs: { name: true } },
-          SidebarItem({
+          SidebarFixedItem({
             id: PAGES.CONNECT,
             dispatch,
             icon: CONNECT_ICON,
             isSelected: PAGES.CONNECT === selectedPage,
             name: "Connect",
-            selectable: true,
           })
         ),
         dom.hr(),
         this.renderDevices()
       )
     );
   }
 }
new file mode 100644
--- /dev/null
+++ b/devtools/client/aboutdebugging-new/src/components/sidebar/SidebarFixedItem.css
@@ -0,0 +1,31 @@
+/* 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/. */
+
+/*
+ * Layout of a fixed sidebar item
+ *
+ *  +--------+----------------+
+ *  | Icon   | Name           |
+ *  +--------+----------------+
+ */
+
+.sidebar-fixed-item {
+  align-items: center;
+  border-radius: 2px;
+  display: grid;
+  grid-template-columns: 34px 1fr;
+  font-size: 16px;
+  height: 48px;
+  padding-inline-end: 10px;
+  padding-inline-start: 10px;
+}
+
+.sidebar-fixed-item__icon {
+  fill: currentColor;
+  height: 24px;
+  margin-inline-end: 9px;
+  width: 24px;
+  -moz-context-properties: fill;
+}
+
new file mode 100644
--- /dev/null
+++ b/devtools/client/aboutdebugging-new/src/components/sidebar/SidebarFixedItem.js
@@ -0,0 +1,66 @@
+/* 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/. */
+
+"use strict";
+
+const { PureComponent, createFactory } = require("devtools/client/shared/vendor/react");
+const dom = require("devtools/client/shared/vendor/react-dom-factories");
+const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
+
+const SidebarItem = createFactory(require("./SidebarItem"));
+
+const Actions = require("../../actions/index");
+
+/**
+ * This component displays a fixed item in the Sidebar component.
+ */
+class SidebarFixedItem extends PureComponent {
+  static get propTypes() {
+    return {
+      dispatch: PropTypes.func.isRequired,
+      icon: PropTypes.string.isRequired,
+      id: PropTypes.string.isRequired,
+      isSelected: PropTypes.bool.isRequired,
+      name: PropTypes.string.isRequired,
+      runtimeId: PropTypes.string,
+    };
+  }
+
+  render() {
+    const {
+      dispatch,
+      id,
+      icon,
+      isSelected,
+      name,
+      runtimeId,
+    } = this.props;
+
+    return SidebarItem(
+      {
+        isSelected,
+        selectable: true,
+        className: "sidebar-fixed-item",
+        onSelect: () => {
+          dispatch(Actions.selectPage(id, runtimeId));
+        }
+      },
+      dom.img(
+        {
+          className: "sidebar-fixed-item__icon",
+          src: icon,
+        }
+      ),
+      dom.span(
+        {
+          className: "ellipsis-text",
+          title: name,
+        },
+        name
+      )
+    );
+  }
+}
+
+module.exports = SidebarFixedItem;
--- a/devtools/client/aboutdebugging-new/src/components/sidebar/SidebarItem.css
+++ b/devtools/client/aboutdebugging-new/src/components/sidebar/SidebarItem.css
@@ -4,48 +4,28 @@
 
 .sidebar-item {
   /* Import css variables from common.css */
   --sidebar-text-color: var(--in-content-category-text);
   --sidebar-selected-color: var(--in-content-category-text-selected);
   --sidebar-background-hover: var(--in-content-category-background-hover);
 }
 
-/*
- * Layout of a sidebar item
- *
- *  +--------+----------------+---------------------------+
- *  | Icon   | Name           | Connect button (optional) |
- *  +--------+----------------+---------------------------+
- */
 .sidebar-item {
-  align-items: center;
+  color: var(--sidebar-text-color);
   border-radius: 2px;
-  color: var(--sidebar-text-color);
-  display: grid;
-  grid-template-columns: 34px auto max-content;
-  font-size: 16px;
-  height: 48px;
   padding-inline-end: 10px;
   padding-inline-start: 10px;
   transition: background-color 150ms;
   -moz-user-select: none;
 }
 
 .sidebar-item:not(.sidebar-item--selectable) {
   opacity: 0.5;
 }
 
 .sidebar-item--selectable:hover {
   background-color: var(--sidebar-background-hover);
 }
 
-.sidebar-item__icon {
-  fill: currentColor;
-  height: 24px;
-  margin-inline-end: 9px;
-  width: 24px;
-  -moz-context-properties: fill;
-}
-
 .sidebar-item--selected {
   color: var(--sidebar-selected-color);
 }
--- a/devtools/client/aboutdebugging-new/src/components/sidebar/SidebarItem.js
+++ b/devtools/client/aboutdebugging-new/src/components/sidebar/SidebarItem.js
@@ -3,61 +3,46 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 const { PureComponent } = require("devtools/client/shared/vendor/react");
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
 const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
 
-const Actions = require("../../actions/index");
-
 /**
- * This component displays an item of the Sidebar component.
+ * This component is used as a wrapper by items in the sidebar.
  */
 class SidebarItem extends PureComponent {
   static get propTypes() {
     return {
-      connectComponent: PropTypes.any,
-      dispatch: PropTypes.func.isRequired,
-      icon: PropTypes.string.isRequired,
-      id: PropTypes.string.isRequired,
+      children: PropTypes.arrayOf(PropTypes.element).isRequired,
+      className: PropTypes.string,
       isSelected: PropTypes.bool.isRequired,
-      name: PropTypes.string.isRequired,
-      runtimeId: PropTypes.string,
       selectable: PropTypes.bool.isRequired,
+      onSelect: PropTypes.func.isRequired,
     };
   }
 
   onItemClick() {
-    const { id, dispatch, runtimeId } = this.props;
-    dispatch(Actions.selectPage(id, runtimeId));
+    this.props.onSelect();
   }
 
   render() {
-    const { connectComponent, icon, isSelected, name, selectable } = this.props;
+    const {children, className, isSelected, selectable } = this.props;
 
     return dom.li(
       {
         className: "sidebar-item js-sidebar-item" +
+                   (className ? ` ${className}` : "") +
                    (isSelected ?
                       " sidebar-item--selected js-sidebar-item-selected" :
                       ""
                    ) +
                    (selectable ? " sidebar-item--selectable" : ""),
         onClick: selectable ? () => this.onItemClick() : null
       },
-      dom.img({
-        className: "sidebar-item__icon",
-        src: icon,
-      }),
-      dom.span(
-        {
-          className: "ellipsis-text",
-          title: name,
-        },
-        name),
-      connectComponent ? connectComponent : null
+      children
     );
   }
 }
 
 module.exports = SidebarItem;
new file mode 100644
--- /dev/null
+++ b/devtools/client/aboutdebugging-new/src/components/sidebar/SidebarRuntimeItem.css
@@ -0,0 +1,17 @@
+/* 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/. */
+
+/*
+ * Layout of a runtime sidebar item
+ *
+ *  +--------+----------------+---------------------------+
+ *  | Icon   | Runtime name   | Connect button            |
+ *  +--------+----------------+---------------------------+
+ */
+
+.sidebar-runtime-item {
+  align-items: center;
+  display: grid;
+  grid-template-columns: 1fr max-content;
+}
new file mode 100644
--- /dev/null
+++ b/devtools/client/aboutdebugging-new/src/components/sidebar/SidebarRuntimeItem.js
@@ -0,0 +1,92 @@
+/* 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/. */
+
+"use strict";
+
+const { createFactory, PureComponent } = require("devtools/client/shared/vendor/react");
+const dom = require("devtools/client/shared/vendor/react-dom-factories");
+const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
+
+const FluentReact = require("devtools/client/shared/vendor/fluent-react");
+const Localized = createFactory(FluentReact.Localized);
+
+const SidebarItem = createFactory(require("./SidebarItem"));
+const Actions = require("../../actions/index");
+
+/**
+ * This component displays a runtime item of the Sidebar component.
+ */
+class SidebarRuntimeItem extends PureComponent {
+  static get propTypes() {
+    return {
+      dispatch: PropTypes.func.isRequired,
+      icon: PropTypes.string.isRequired,
+      id: PropTypes.string.isRequired,
+      isConnected: PropTypes.bool.isRequired,
+      isSelected: PropTypes.bool.isRequired,
+      name: PropTypes.string.isRequired,
+      runtimeId: PropTypes.string.isRequired,
+    };
+  }
+
+  renderConnectButton() {
+    return Localized(
+      {
+        id: "about-debugging-sidebar-item-connect-button"
+      },
+      dom.button(
+        {
+          className: "sidebar-item__connect-button",
+          onClick: () => {
+            const { dispatch, runtimeId } = this.props;
+            dispatch(Actions.connectRuntime(runtimeId));
+          }
+        },
+        "Connect"
+      )
+    );
+  }
+
+  render() {
+    const {
+      dispatch,
+      icon,
+      id,
+      isConnected,
+      isSelected,
+      name,
+      runtimeId,
+    } = this.props;
+
+    return SidebarItem(
+      {
+        isSelected,
+        selectable: isConnected,
+        className: "sidebar-runtime-item",
+        onSelect: () => {
+          dispatch(Actions.selectPage(id, runtimeId));
+        }
+      },
+      dom.span(
+        { className: "ellipsis-text" },
+        dom.img(
+          {
+            className: "sidebar-runtime-item__icon " +
+             `${isConnected ? "sidebar-runtime-item__icon--connected" : "" }`,
+            src: icon,
+          }
+        ),
+        dom.span(
+          {
+            title: name,
+          },
+          ` ${name}`
+        ),
+      ),
+      !isConnected ? this.renderConnectButton() : null
+    );
+  }
+}
+
+module.exports = SidebarRuntimeItem;
--- a/devtools/client/aboutdebugging-new/src/components/sidebar/moz.build
+++ b/devtools/client/aboutdebugging-new/src/components/sidebar/moz.build
@@ -1,12 +1,14 @@
 # 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(
-    'DeviceSidebarItemAction.css',
-    'DeviceSidebarItemAction.js',
     'Sidebar.css',
     'Sidebar.js',
+    'SidebarFixedItem.css',
+    'SidebarFixedItem.js',
     'SidebarItem.css',
     'SidebarItem.js',
+    'SidebarRuntimeItem.css',
+    'SidebarRuntimeItem.js',
 )