Closed Bug 1419076 Opened 7 years ago Closed 7 years ago

Make the console sidebar resizable

Categories

(DevTools :: Console, enhancement, P1)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: nchevobbe, Assigned: mpark)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1419076 - Make the console sidebar resizable.
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Blocks: 1380501
Severity: normal → enhancement
Priority: -- → P2
Bug 1419075 should land soon, we can start working on this bug.
Assignee: nobody → mpark
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8931136 [details]
Bug 1419076 - Make the console sidebar resizable.

https://reviewboard.mozilla.org/r/202218/#review207808

::: devtools/client/webconsole/new-console-output/components/OutputWrapper.js:31
(Diff revision 1)
> +    let mainWrapper = dom.div({
> +      className: "webconsole-main-output"
> +    }, filterBar, consoleOutput);
> +
> +    return SplitBox({
> +      className: "webconsole-output-wrapper",
> +      splitterSize: 1,
> +      endPanelControl: true,
> +      startPanel: mainWrapper,
> +      endPanel: sidebarVisible && sideBar,
> +      initialSize: "200px",
> +      vert: true,
> +    });
> +  }
> +}
> +
> +function mapStateToProps(state, props) {
> +  return {
> +    sidebarVisible: state.ui.sidebarVisible,
> +  };
> +}

I am concerned about adding a component only for the sake of the splitter.
It also defeats the purpose of the grid we set in the previous patch.
Would it be possible in some way to only add a Draggable component (https://searchfox.org/mozilla-central/source/devtools/client/shared/components/splitter/Draggable.js), which we would show only if sideBarVisible is true ? And have the https://searchfox.org/mozilla-central/source/devtools/client/shared/components/splitter/Draggable.js#17 onStop set the width of the sideBar somewhere.


it would go like that : 
- the console is displayed without the sidebar
- the sideBarToggle action is dispatched
- the reducer flips the sidebarVisible property
- we do render the draggable component as well as the sidebar
- the draggable dispatches a "sidebarResized" action when its onMove property is called
- the reducer listen for this action, and return a new `sideBarWidth` property (and later, persist it in prefs)
- the sideBar component is connected to the store to react to a `sidebarWidth` changes, and render accordingly div({style: {width: this.props.sidebarWidth}}). This way , both the draggable component and the sidebar "move".

We would only need to maybe set the initial min width of the sidebar directly in JS.

What do you think about this Mike ?

If we realize that's not doable, or that it's to much work in comparison of simply use the SplitterBox, we can always revert to this patch.

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:202
(Diff revision 1)
>            consoleOutput,
> +          filterBar,

I think consoleOutput and filterBar can be passed as children : 

`OutputWrapper({sideBar}, filterBar, consoleOutput)`
Attachment #8931136 - Flags: review?(nchevobbe) → review-
We could avoid having to create new actions/reducers by putting the draggable in the Sidebar component, and using the Sidebar's state to keep track of the width. This would basically be reimplementing SplitBox exactly though.

Will the sidebar be always on the right side, or will we put it at the bottom when the panel is in a vertical configuration (i.e. docked to the side)?
(In reply to Mike Park [:mparkms] from comment #4)
> We could avoid having to create new actions/reducers by putting the
> draggable in the Sidebar component, and using the Sidebar's state to keep
> track of the width. This would basically be reimplementing SplitBox exactly
> though.
> 
> Will the sidebar be always on the right side, or will we put it at the
> bottom when the panel is in a vertical configuration (i.e. docked to the
> side)?

It might go to the bottom at some point, not sure.
I am fine with using the SplitBox as long as we can keep the grid layout defined in https://bugzilla.mozilla.org/show_bug.cgi?id=1136299.

Can we move the SideBar in its own component, where it would render itselft using the splitbox:

SplitBox({
  classNames: ["sidebar"],  
  endPanel: dom.aside({…}, 
  "Sidebar wip")
})

Maybe that would work, and we only need to switch the css placement rules from the aside to the top-level Splitbox .sidebar.

Does it sounds good to you ?
Comment on attachment 8931136 [details]
Bug 1419076 - Make the console sidebar resizable.

https://reviewboard.mozilla.org/r/202218/#review208186

This seems good ! Tested and works fine when docked to bottom or to the right.
100px seems a bit small, but we can handle that in another bug to switch to a vertical layout.

::: devtools/client/webconsole/new-console-output/components/SideBar.js:31
(Diff revision 2)
> +          minSize: "100px",
> +          // The left side of the console cannot shrink smaller than 400px. Set max-width
> +          // like this to prevent the sidebar from expanding to the right, off the page,
> +          // and breaking the draggable.
> +          maxSize: "calc(100vw - 400px)",

can't this be set in the CSS ?
Attachment #8931136 - Flags: review?(nchevobbe) → review+
Comment on attachment 8931136 [details]
Bug 1419076 - Make the console sidebar resizable.

https://reviewboard.mozilla.org/r/202218/#review208200

perfect
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df9a033ef32d
Make the console sidebar resizable. r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/df9a033ef32d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: