Vertical layout scopes pane by wldcordeiro · Pull Request #1575 · firefox-devtools/debugger

wldcordeiro · 2016-12-31T00:24:08Z

Associated Issue: #1359

Summary of Changes

Rework RightSidebar into an InformationPanes component that renders based on layout type
Hide the search hint in Sources when in vertical layout
Refactor the collapse button into a component for reuse.

Test Plan

Example test plan:

Open the debugger
Resize the debugger window to > 700px
View that scopes pane is on the right side of the lower pane
View that the search hint is gone from the sources sidebar

Screenshots/Videos (OPTIONAL)

…able for vertical layout. Refactor collapse button code into a PaneCollapseButton component to be reused for vertical layout.

…ent layouts for vertical and horizontal.

jasonLaster

I read this quickly. My first impression is that you came up with many nice naming / components.

I'll study it more later.

jasonLaster

Lots of comments, but in reality this is a huge step forward. Thanks for doing this. I'd love to formally launch vertical layout.

You and @kdzwinel will get a lot of credit!

jasonLaster · 2016-12-31T05:08:30Z

 const Breakpoints = React.createFactory(require("./Breakpoints"));
 const Expressions = React.createFactory(require("./Expressions"));

+let { SplitBox } = require("devtools-modules");


nit: const SplitBox = createFactory(require("devtools-modules").SplitBox)

jasonLaster · 2016-12-31T05:09:39Z


+    let scopesContent = {
+      header: L10N.getStr("scopes.header"),
+      component: Scopes, shouldOpen: isPaused };


nit: shouldOpen could be on its own line

jasonLaster · 2016-12-31T05:11:45Z

+
+  renderHorizontalLayout() {
+    return Accordion({
+      items: this.getItems()


this might be a stretch, but I could see separate pane functions:

renderHorizontalLayout() { return Accordion({ items: [ this.scopesPane(), this.framesPane(), ] }) }

jasonLaster · 2016-12-31T05:13:08Z

+
    return dom.div({ className: "source-header" },
-      this.renderToggleButton("start", !this.props.startPanelCollapsed),
+      PaneCollapseButton({


Would this be helpful if we added a collapse button for bottom bar?

@jasonLaster Yeah, the button was refactored for that and would go in the CommandBar component for the bottom pane. I had that coded up but with the vertical collapse not yet finished I stashed that till later.

jasonLaster · 2016-12-31T05:14:36Z

+function PaneCollapseButton({ position, collapsed, horizontal, handleClick }) {
+  return React.DOM.div({
+    className: classnames(`toggle-button-${position}`, {
+      collapsed, vertical: horizontal != null ? !horizontal : false }),


nit: each field can be on its own line

jasonLaster · 2016-12-31T05:16:03Z

+function PaneCollapseButton({ position, collapsed, horizontal, handleClick }) {
+  return React.DOM.div({
+    className: classnames(`toggle-button-${position}`, {
+      collapsed, vertical: horizontal != null ? !horizontal : false }),


vertical: horizontal != null ? !horizontal : false can be simplified :)

Perhaps:

we pass in vertical to be consistent internally

we use a default value in the params: PaneCollapseButton({ vertical = false })

jasonLaster · 2016-12-31T05:16:45Z

+  }, Svg("togglePanes"));
+}
+
+module.exports = PaneCollapseButton;


nit: PaneCollapseButton could be PaneToggleButton

jasonLaster · 2016-12-31T05:17:46Z

+        L10N.getFormatStr("sources.search",
+          formatKeyShortcut(`CmdOrCtrl+${L10N.getStr("sources.search.key")}`))
+      );
+    }


do you think it would be nice if we had a renderShortcut method to hide this?

jasonLaster · 2016-12-31T05:21:49Z

 }

-const RightSidebar = React.createClass({
+const InformationPanes = React.createClass({


Hmm, so the more i think about it the more I wonder if InformationPanes is too vague.

My beef with information is largely that it could be applied to anything :)

One thought is that we could refer to the left and right sidebars as primary and secondary panes. At the moment, the left sidebar is just sources, but if I were to bet, it would get other items over time.

I don't have a particular affinity for the name either, something more descriptive would be good. 👍

jasonLaster · 2016-12-31T05:23:09Z

-        Accordion({
-          items: this.getItems()
-        })
+        paneContents


nit: this could be a ternary here

jasonLaster

👍 Landing it!

jasonLaster · 2017-01-02T05:00:14Z

added a couple of small stylistic tweaks, but this was so much great work.

It was fun to work on this with you, lots of naming and moving things around

wldcordeiro · 2017-01-02T05:53:53Z

@jasonLaster awesome! It's great landing features like this. Glad to work on it.

* Refactor RightSidebar component into InformationPanes to be more suitable for vertical layout. Refactor collapse button code into a PaneCollapseButton component to be reused for vertical layout. * Rework the RightSidebar into InformationPanes that renders two different layouts for vertical and horizontal. * Code clean-up from review. * Rename InformationPanes to SecondaryPanes * small tweaks

wldcordeiro added 2 commits December 30, 2016 17:04

Refactor RightSidebar component into InformationPanes to be more suit…

03238ac

…able for vertical layout. Refactor collapse button code into a PaneCollapseButton component to be reused for vertical layout.

Rework the RightSidebar into InformationPanes that renders two differ…

d714869

…ent layouts for vertical and horizontal.

jasonLaster reviewed Dec 31, 2016

View reviewed changes

wldcordeiro force-pushed the vertical-layout-scopes-pane branch from 28104a1 to d20d718 Compare December 31, 2016 07:19

Code clean-up from review.

0cf4f21

wldcordeiro force-pushed the vertical-layout-scopes-pane branch from d20d718 to 0cf4f21 Compare December 31, 2016 07:37

wldcordeiro and others added 2 commits December 31, 2016 17:45

Rename InformationPanes to SecondaryPanes

5e2395e

small tweaks

cbaf867

jasonLaster approved these changes Jan 2, 2017

View reviewed changes

jasonLaster merged commit 81048c4 into firefox-devtools:master Jan 2, 2017

wldcordeiro deleted the vertical-layout-scopes-pane branch January 2, 2017 05:53

James300 mentioned this pull request Sep 22, 2017

Scroll bar is not ignored in vertical mode calculations #4127

Merged

Uh oh!

Conversation

wldcordeiro commented Dec 31, 2016

Summary of Changes

Test Plan

Screenshots/Videos (OPTIONAL)

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wldcordeiro Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

jasonLaster commented Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wldcordeiro commented Jan 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wldcordeiro Dec 31, 2016 •

edited

Loading

jasonLaster commented Jan 2, 2017 •

edited

Loading