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 489591 5a54f333b855138687b5b71e33c1c6144fc7fbe5
parent 489590 5f44f57c4ae734a8c9ac490c4e520c703365beae
child 489592 a871d40e8802cc8f049548ba40ccebe550a69c83
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersjdescottes, daisuke
bugs1494543
milestone64.0a1
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',
 )