1419078 - Add a close button in the sidebar

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

The button should call the toggleSideBar action.

Comment 1

[Mike Park [:mparkms]]

Attachment: Bug 1419078 - Add a close button to the console sidebar.

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8933002 [details]
Bug 1419078 - Add a close button to the console sidebar.

https://reviewboard.mozilla.org/r/204000/#review209758

This looks good overall.
I have a few comment about the CSS though.
Also, we should try to align the close button of the toolbox with the one in the sidebar (http://prntscr.com/hh9nky)

::: devtools/client/themes/webconsole.css:1223
(Diff revision 1)
> +.sidebar-contents {
> +  grid-row: 2 / 3;
> +}
> +
> +.sidebar-close .sidebar-close-button {
> +  float: right;

we can avoid floating things. 
Let's put a display flex on the toolbar, so we can then use `justify-content: end` (see https://codepen.io/nchevobbe/pen/QOJYye)

::: devtools/client/webconsole/new-console-output/components/SideBar.js:35
(Diff revision 1)
>    render() {
>      let {
>        sidebarVisible,
>      } = this.props;
>  
> +    let endPanel = dom.div({

we should use dom.aside

::: devtools/client/webconsole/new-console-output/components/SideBar.js:38
(Diff revision 1)
>      } = this.props;
>  
> +    let endPanel = dom.div({
> +      className: "sidebar-wrapper"
> +    },
> +      dom.div({

we could use dom.header here

::: devtools/client/webconsole/new-console-output/components/SideBar.js:39
(Diff revision 1)
>  
> +    let endPanel = dom.div({
> +      className: "sidebar-wrapper"
> +    },
> +      dom.div({
> +        className: "devtools-toolbar sidebar-close"

it may be used for other needs in the future (a filter bar for example), so this should be renamed to something more neutral (webconsole-sidebar-toolbar ?)

Comment 3

[Mike Park [:mparkms]]

Comment on attachment 8933002 [details]
Bug 1419078 - Add a close button to the console sidebar.

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8933002 [details]
Bug 1419078 - Add a close button to the console sidebar.

https://reviewboard.mozilla.org/r/204000/#review209860

Looks good, thanks Mike !
Do you think we could add a mocha test to better start etsting the sidebar ?
For now it would be just making sure that we do show a close button, and that clicking on it do call the expected function.
You can have a look at the filter bar tests to take some inspiration

Comment 5

[Mike Park [:mparkms]]

Comment on attachment 8933002 [details]
Bug 1419078 - Add a close button to the console sidebar.

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

Comment 6

[Mike Park [:mparkms]]

Comment on attachment 8933002 [details]
Bug 1419078 - Add a close button to the console sidebar.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ffb0df035e0d
Add a close button to the console sidebar. r=nchevobbe

Comment 8

[Dorel Luca [:dluca]]

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