1449024 - Extend Tabs component to allow a toggle button beside the tabs

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

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.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1449024 - Extend Tabs component to allow a toggle button beside the tabs.

Review commit: https://reviewboard.mozilla.org/r/231468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231468/

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8962616 [details]
Bug 1449024 - Extend Tabs component to allow a toggle button beside the tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231468/diff/1-2/

Comment 5

[Code Review Bot [:reviewbot]]

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 6

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8962616 [details]
Bug 1449024 - Extend Tabs component to allow a toggle button beside the tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231468/diff/2-3/

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d782d5d22f4d
Extend Tabs component to allow a toggle button beside the tabs. r=Honza

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d782d5d22f4d