Closed Bug 1449024 Opened 2 years ago Closed 2 years ago

Extend Tabs component to allow a toggle button beside the tabs

Categories

(DevTools :: Framework, enhancement, P3)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Add support in our simple Tabs widget (https://searchfox.org/mozilla-central/source/devtools/client/shared/components/tabs/Tabs.js) to allow a placement of a sidebar toggle button. This work is part of the UX/Photon polish for network monitor and inspector https://docs.google.com/document/d/1CsxazGLKlZagc1rCt-4j_gqh5WEQLCxN6VUjd5AUTiM/edit. In the inspector, this will allow us to render the 3 pane toggle button next to the tabs as seen in the bottom images of https://mozilla.invisionapp.com/share/Z3F7OGCTK#/screens/284159829.

We would also make this a UX pattern for the network monitor sidebar by moving the toggle button next to the tab toolbar. This is described in the TODO of https://docs.google.com/document/d/1CsxazGLKlZagc1rCt-4j_gqh5WEQLCxN6VUjd5AUTiM/edit.
Blocks: 1446503
Comment on attachment 8962616 [details]
Bug 1449024 - Extend Tabs component to allow a toggle button beside the tabs.

https://reviewboard.mozilla.org/r/231468/#review237124

`SidebarToggle` component can't be included in `Tabs`. Tabs is used by JSONViewer and loaded using RequireJS async - requires using `define`.

It's better to define a new Component `Sidebar` -> `devtools/client/shared/components/Sidebar` (along side the existing `SidebarToggle`) and put the new functionality into it. It's better anyway since `Tabs` component is generic and `Sidebar` will nicely correspond and utilize `SidebarToggle` component. The toggle button will be expected feature of such component.

Honza

::: devtools/client/shared/components/tabs/TabBar.js:39
(Diff revision 1)
> +      renderToggleButton: PropTypes.shape({
> +        collapsed: PropTypes.bool.isRequired,
> +        collapsePaneTitle: PropTypes.string.isRequired,
> +        expandPaneTitle: PropTypes.string.isRequired,
> +        onClick: PropTypes.func.isRequired,
> +      }),

Please create a comment for every prop.

::: devtools/client/shared/components/tabs/Tabs.js:15
(Diff revision 1)
> -  const { Component } = require("devtools/client/shared/vendor/react");
> +  const { Component, 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 { findDOMNode } = require("devtools/client/shared/vendor/react-dom");
>  
> +  const SidebarToggle = createFactory(require("devtools/client/shared/components/SidebarToggle"));

This breaks JSONViewer since SidebarToggle module doesn't support async loading (is not enclosed in `define` method)

See also my general comment.
Attachment #8962616 - Flags: review?(odvarko)
Comment on attachment 8962616 [details]
Bug 1449024 - Extend Tabs component to allow a toggle button beside the tabs.

https://reviewboard.mozilla.org/r/231468/#review237146
Blocks: 1449100
Comment on attachment 8962616 [details]
Bug 1449024 - Extend Tabs component to allow a toggle button beside the tabs.

https://reviewboard.mozilla.org/r/231468/#review238596


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/shared/components/Sidebar.js:8
(Diff revision 2)
> + * 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");

Error: 'dom' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/shared/components/Sidebar.js:32
(Diff revision 2)
> +        collapsePaneTitle: PropTypes.string.isRequired,
> +        expandPaneTitle: PropTypes.string.isRequired,
> +        onClick: PropTypes.func.isRequired,
> +      }),
> +      tabActive: PropTypes.number,
> +    }

Error: Missing semicolon. [eslint: semi]

::: devtools/client/shared/components/Sidebar.js:88
(Diff revision 2)
> +      )
> +    );
> +  }
> +}
> +
> +module.exports= Sidebar;

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: devtools/client/shared/components/tabs/Tabs.js:336
(Diff revision 2)
>          })
>        ) : null;
>  
> +      // Get the sidebar toggle button if a renderSidebarToggle function is provided.
> +      let sidebarToggle =  this.props.renderSidebarToggle ?
> +        this.props.renderSidebarToggle() : null

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8962616 [details]
Bug 1449024 - Extend Tabs component to allow a toggle button beside the tabs.

https://reviewboard.mozilla.org/r/231468/#review238834

Looks great to me, thanks for working on this!
R+ assuming try is green
Honza
Attachment #8962616 - Flags: review?(odvarko) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d782d5d22f4d
Extend Tabs component to allow a toggle button beside the tabs. r=Honza
https://hg.mozilla.org/mozilla-central/rev/d782d5d22f4d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.