Garbee/breakpoints by Garbee · Pull Request #1712 · firefox-devtools/debugger

Garbee · 2017-01-14T23:12:14Z

Associated Issue: #1665

Summary of Changes

Sorry, pretty massive changes in this one. Plenty to discuss.

UX Side

Moved "Pause on Next Exception" toggle and the exception breakpoints under the breakpoints header to get them off the CommandBar itself.
That change allows for the CommandBar to be rendered only when paused, where most of the actions are needed.
Pause on next execution is a button trigger.
Exceptions are put under a collapsible node as distinct radios. This provides better clarity to developers as to what they are switching between and reduces the clicks to get to different modes. Upon selecting a different mode, the collapse header updates to show the current mode so even when collapsed you know which mode you're in.
Toggle all breakpoints button moved from collapse header into a toggle area of the breakpoint list itself.
The user breakpoints header also adds a "Remove all" button to quickly remove all of a users custom breakpoints if desired.
- The remove all button triggers a confirm dialog. We should figure out the best way to handle this in our UX. This confirm is added due to having heard some developers in the past with Chrome DevTools having loads of breakpoints and accidentally losing them at different points. We can either temporarily store a copy and provide an "undo" action for a few seconds or do an inline confirm UX. If we can have people lose a lot of work easily, we should try to let them undo it or force them to confirm the action.

Technical side (JSX)

With this I experimented with using JSX for the components. All that was needed to let us do this is adding the right babel transformer to our list.

Pros

Nesting is easier to follow since we aren't just providing a parameter list.
Output expectations are very clear.
JSX is what most React developers use since it is the default. So it is more natural for fresh contributors to get help with if they need it.

Cons

Code style can seem fugly.
- Braces for using variables in rendering the component.
- Inline anonymous functions look weird. Moved them to defined constants.

Test Plan

Fairly standard testing run-through with the exceptions page and toggling a few actions on the todomvc demo. Just to verify the functionality works as it used to. Most changes here as far as the frontend are on the UX side over the technical side.

Screenshots/Videos

…s display as block. Re-add no breakpoints message.

jasonLaster · 2017-01-14T23:33:11Z

Thanks @Garbee!

@clarkbw, @wldcordeiro, @bomsy, John and I discussed using this branch to discuss some research questions:

do we like the JSX style (what questions do we have there)
do we like some of these UX changes (what questions do we have there)

With the UX changes, I'd like to ideally test the change with UR and AB (node electron app)
With JSX, I'd like to make a decision after we cut the new release and come up with a plan for how we'll incrementally upgrade with a style doc that can evolve.

Lastly, we should rope in the larger team before we pull in JSX

jasonLaster

left some style comments

jasonLaster · 2017-01-14T23:33:43Z

+        disabled={ isWaitingOnBreak }
+        onClick={ breakOnNext }
+      >
+        <Svg name="pause" />


jasonLaster · 2017-01-14T23:34:03Z

+        onClick={ breakOnNext }
+      >
+        <Svg name="pause" />
+        { isWaitingOnBreak ?


maybe this is pulled up to a variable... Also we should have L10N strings eventually here

Yup. L10N needs to get sprinkled around. I just left it out for the initial review to keep what we are looking at more concise.

jasonLaster · 2017-01-14T23:34:26Z

+        <label className="breakpoint" key={ fromMode.mode }>
+          <input
+            type="radio"
+            onChange={ this.pauseExceptionModeToggled }


do people use spaces here? ={

Typically not from what I see around in the docs and other projects. When the event handlers were inlined those had them for ease of reading so I put theme everywhere for consistency. Now that those handlers are seeming like we're going to keep them in variables, the spaces here are useless.

jasonLaster · 2017-01-14T23:35:00Z

+    return (
+      <details>
+        <summary className="_header">
+          { `Exceptions - Pausing on: ${currentMode.headerLabel}` }


eventually, this would move to L10N

jasonLaster · 2017-01-14T23:35:23Z

+        <summary className="_header">
+          { `Exceptions - Pausing on: ${currentMode.headerLabel}` }
+          </summary>
+        { this.props.exceptionPauseModes.map(_createToggle) }


I think this is nice, but I'm still a little reluctant about curlies :)

Curlies are a tradeoff for overall clarity in the nesting.

jasonLaster · 2017-01-14T23:35:43Z

+      if (input) {
+        input.indeterminate = isIndeterminate;
+      }
+    };


I like pulling these functions out of the curlies. I could see the markup becoming pretty hard to follow with these.

Personally, I prefer the old JS style for these in line functions, but ... what can you do :)

I don't think we have a good alternative to add refs/event handlers any other way. :/ So, this is just the way things are with JSX if we go this route.

wldcordeiro · 2017-01-15T04:08:26Z

Minor comment: If we start going JSX and this lands let's open a tracking issue for JSX converting the rest of the codebase.

Garbee added 18 commits January 11, 2017 06:51

Move commandbar into breakpoint area.

bd638a0

Remove top level breakpoint toggle

ee85951

Only show step buttons when paused.

e070809

Simplify rendering breakpoint toggles.

e5940aa

Initial radio toggles for exception breakpoints.

0df9bb9

Add details to keep hide away toggles when not needed. All breakpoint…

d82ec92

…s display as block. Re-add no breakpoints message.

Do not render exception pausing button in CommandBar

2008f51

Don't render pause in CommandBar. Buttons are always active when shown.

151dd87

Move CommandBar back into top level

88f0e45

Minor style updates.

3679f5f

Fix size check on breakpoints existing.

547bd9a

Fix linting issues.

e3086cc

Finish feature functionality for breakpoints refresh.

dadf39d

Convert to JSX for CommandBar.

4803039

Merge upstream

8c3b6c4

Remove uneeded constant declaration.

93e88c4

Converted breakpoints to JSX

cb2ff23

Don't use inline anonymous function bindings for event handlers.

470336c

Garbee mentioned this pull request Jan 14, 2017

Command Bar redesign #1665

Closed

Import svg

26b083c

jasonLaster reviewed Jan 14, 2017

View reviewed changes

jasonLaster mentioned this pull request Jan 18, 2017

Research adopting JSX #1747

Closed

jasonLaster closed this Jan 18, 2017

Navigation Menu

Garbee/breakpoints - #1712

Garbee/breakpoints#1712
Garbee wants to merge 19 commits into
firefox-devtools:masterfrom
Garbee:garbee/breakpoints

Garbee commented Jan 14, 2017

Uh oh!

jasonLaster commented Jan 14, 2017 •

edited

Loading

Uh oh!

jasonLaster left a comment •

edited

Loading

Uh oh!

jasonLaster Jan 14, 2017

Uh oh!

jasonLaster Jan 14, 2017

Uh oh!

Garbee Jan 15, 2017

Uh oh!

jasonLaster Jan 14, 2017

Uh oh!

Garbee Jan 15, 2017

Uh oh!

jasonLaster Jan 14, 2017

Uh oh!

jasonLaster Jan 14, 2017

Uh oh!

Garbee Jan 15, 2017

Uh oh!

jasonLaster Jan 14, 2017

Uh oh!

Garbee Jan 15, 2017

Uh oh!

wldcordeiro commented Jan 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Garbee commented Jan 14, 2017

Summary of Changes

UX Side

Technical side (JSX)

Pros

Cons

Test Plan

Screenshots/Videos

Uh oh!

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

Uh oh!

Uh oh!

jasonLaster left a comment • 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!

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 commented Jan 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jasonLaster commented Jan 14, 2017 •

edited

Loading

jasonLaster left a comment •

edited

Loading