Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #8049 Make WhyPaused more visible #8163

Merged
merged 17 commits into from Apr 17, 2019

Conversation

Projects
None yet
5 participants
@derek-li
Copy link
Contributor

commented Mar 26, 2019

Fixes #8049

Summary of Changes:

  • Make the WhyPaused message more visible
  • Move WhyPaused back to original position for added visibility
  • Reduce visual noise while stepping through the debugger

Demo:
WhyPaused moved to top of the secondary pane and higher contrast
8049 1

Reduced vertical jumpiness (if anybody has suggestions for different solutions I'd love to hear them)
8049 2

In progress:

  • Show whyPause when end panel is collapsed
  • Styling

derek-li added some commits Mar 26, 2019

Fix for #8049 (WIP)
- Moved WhyPaused component to previous location (top of pane frames) for more visibility
- Reduced vertical jumpiness on re-renders/updates
- Minor changes to contrast
Revert WhyPaused
-Need to use PureComponent to prevent infinite updates

@darkwing darkwing changed the title Issue #8049 Make WhyPaused more visible (WIP_ Issue #8049 Make WhyPaused more visible Mar 26, 2019

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

I see why you've attempted a 100ms delay -- cutting down on sidebar noise is important. This PR is a really good kicking off point. A few things:

  1. I'm wondering if a different background color for that block would be good. Maybe a light yellowish color to distinguish it from other areas of the debugger.

  2. I'd love @digitarald's insight and ideas. I know we've considered a page overlay but that's a huge leap; his insight on the next step would be great!

@digitarald

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

@derek-li this looks great. The 100ms is an excellent idea and seems to work on first sight – and it should still give that instant feedback feeling.

Some thoughts on styling –👍 on David's comment: Could you remove the italic styling and try a more visible styling with yellow background, as shown further down in the photon warnings guide. Left-aligned might also work with a little bit more vertical space (I wonder if mentioning spacing summons @fvsch), maybe with a paused icon next it to provide the paused context.

@fvsch

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

I guess I'm summoned now :P

Our spacing for secondary panes content is generally like this:

.some-container {
  padding: 4px 20px;
}

One way to avoid vertical jumps is to use a min-height with vertical centering and making sure that min-height accommodates 2 lines. That means it might look tall and half-empty when we only show one line, though.

.some-container {
  display: flex;
  flex-direction: column;
  justify-content: center;
  min-height: 44px;
  padding: 4px 20px;
}
@digitarald

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

One way to avoid vertical jumps is to use a min-height with vertical centering and making sure that min-height accommodates 2 lines.

I like that. Its a best effort as some users might have a small enough sidebar to have text run over into 3 lines but it makes it better for most users. The jump from 2 to 1 line is not too jarring in the recording, so I'd leave that up to tweaking when we have something testable.

@fvsch

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Do we want to show an icon to represent the paused state?
Maybe a red octagon (the classic stop sign)?

(We can always add that later.)

@digitarald

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Do we want to show an icon to represent the paused state?

Not sure if 🛑 STOP is the right metaphor here. We do use for pausing in the debugger actions and it should probably also be used for the Debugger toolbar tab when being paused, vs a green glowing breakpoint. Downside of the icon is that it could confuse, so maybe a simple info icon is enough.

Took a stab at it:

image

@fvsch

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

I'm just worried about the "pause" icon because we already use it as an action button, as in "click this pause button to pause JS execution". That could be confusing.

@derek-li

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Downside of the icon is that it could confuse, so maybe a simple info icon is enough

I like the info icon idea. The WhyPaused component exists to explain why the debugger is paused rather than showing that the debugger is paused, so I don't agree with using stop or pause icons.

@digitarald

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

I like the info icon idea. The WhyPaused component exists to explain why the debugger is paused rather than showing that the debugger is paused, so I don't agree with using stop or pause icons.

Agreed! Sounds like we got all the bigger questions answered then.

The end result might (aside from spacing, which I have not mocked up) look like this:

image

… but I let @derek-li prototype and share what he comes up with 👍

@derek-li

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

Here's what I have currently:
cdeaa6c1db74c7b05df6a6ad7e8a3a7e

What are your guys opinion on using a less intrusive yellow for the warning as well as hiding overflow instead of allowing the text to wrap?

derek-li added some commits Apr 2, 2019

derek-li added some commits Apr 3, 2019

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

I like this lighter yellow a lot! As for overflow, I would wrap the text. I wouldn't want to require a user play with the sidebar width just to see a full message.

@derek-li

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

I'm playing with some ideas to render the whyPaused component when the end pane is collapsed, but I don't feel that any of them look great.

Here's the mock-up I think works best, but I'm not satisfied.
8049 2

@digitarald @fvsch Any input?

Also considering leaving this for another issue.

@fvsch

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

@derek-li I think we should first move the WhyPaused component to the top and tweak its design, then think in follow-up bugs about maybe making it a floating UI element when the sidebar is collapsed. I opened #8181 to describe the more general UX issue and possible solutions.

For the WhyPaused style, could we use the same background color as the Console's warning messages? They are defined here:
https://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#27,39
Not sure we want to use the Console's warning text color, though.

derek-li added some commits Apr 5, 2019

@derek-li

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Here's the current iteration. Do you guys have any input on the placement of the info icon?

light:
8049 1

dark:
8049 2

@fvsch why don't we want to use the Console's warning text color?

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

@derek-li We spoke about this a bit and we're thinking one change:

  • Could you remove the light yellow background color and, instead, bold the first line in the error box?

We think that's the next UI to try. 👍

@derek-li

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@darkwing Here's the latest mock-ups for light and dark modes.

Light:
lightmode1

What are you guys thinking for the dark mode?

Dark 1(No background color, no font color)
darkmode3

Dark 2 (No background color)
darkmode1

Dark 3
darkmode2

@fvsch

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

I like Light + Dark1 (the ones without yellow).
That way we keep the top visual priority for the breakpoint/error line highlight in Editor.

@derek-li

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@fvsch @darkwing Let me know if there's anything else you guys want tweaked! Otherwise should be good to merge

return 100;
}

return 0;

This comment has been minimized.

Copy link
@jasonLaster

jasonLaster Apr 11, 2019

Contributor

is this delay new?

This comment has been minimized.

Copy link
@derek-li

derek-li Apr 11, 2019

Author Contributor

No, I haven't changed it since creating the initial PR.

@digitarald

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

@derek-li thanks for iterating on the tweaks, the screenshots help a lot.

One guiding principle for UI decisions is to make it easy for devs to switch between devtools. The paused indicator is a strong visual clue that should not be missed (main goal for this redesign). As Chrome uses a yellow-colored paused indicator, I would default to that as well, so the version using the console warning styling (with yellow background), would be the most fitting.

As I haven't seen the argumentation, I'd be happy to hear the trade offs of using a yellow background vs other options, @fvsch @darkwing ?

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

Thank you for jumping in the discussion @digitarald !

@jasonLaster and I met with @fvsch and @derek-li met last week and I think the conclusion was that the yellow background would take focus away from the DebugLine (in the case of a breakpoint).

If you prefer the yellow background be restored, we can do that as well. I'd like to get this in before it gets too stale :)

@fvsch

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Yellow background, cons:

  • Not exactly a warning.
  • Can distract from putting focus on the highlighted code line.

Pros:

  • Consistent with Chrome DevTools.
  • Can help to make the paused state more obvious when users didn't expect a paused state. (Scenario: user sets a breakpoint and forgets about it, and/or enables "Pause on exceptions" or "Pause on any URL"; user interacts with another panel such as the Inspector and interacts with the page; everything freezes; user hopefully goes to Debugger to figure out what's happening.)

No strong feelings, I'm okay with both styles.

@derek-li

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

I feel we shouldn't differentiate from Chrome unless there are strong reasons to do so.
For now, I'll revert back to the Yellow background

@darkwing darkwing merged commit c2c176c into firefox-devtools:master Apr 17, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@derek-li derek-li deleted the derek-li:issue-#8049 branch Apr 17, 2019

@derek-li

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Appreciate the help from everyone!

@digitarald

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@derek-li thanks for the contribution with code and design thinking. This polish will go a long way to make my debugging life and hopefully others a lot more predictable 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.