Closed Bug 1184644 Opened 4 years ago Closed 4 years ago

Refresh the SideMenu Widget theme

Categories

(DevTools :: CSS and Themes, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 1 open bug)

Details

(Whiteboard: [devtools-ux])

Attachments

(1 file, 9 obsolete files)

Attached image debugger-before.png (obsolete) —
We should probably refresh the sidemenu widget to use our theme colors.
That will help eliminate preprocessor defines and remove the cruft from the old UI.
Attached image debugger-after.png (obsolete) —
Attached image se-before.png (obsolete) —
Attached image se-after.png (obsolete) —
Stephen, I'm mainly concerned about how the debugger sidebar looks, especially the domain headers. What do you think ?
Flags: needinfo?(shorlander)
Summary: Refresh the SideMenu Widget → Refresh the SideMenu Widget theme
Whiteboard: [devtools-ux]
(In reply to Tim Nguyen [:ntim] from comment #4)
> Stephen, I'm mainly concerned about how the debugger sidebar looks,
> especially the domain headers. What do you think ?

I like the idea of defining a color for sidebars, I don't think we should lose the contrast between the sidebar and the tool content area. 

I agree the domain headers could be clearer. Maybe we should remove the background color from the domain header, remove the separator between it and the sources and indent the sources?
Flags: needinfo?(shorlander)
Attached patch WIP (obsolete) — Splinter Review
I still need to get rid of splitview.css and move the remaining stuff to styleeditor.css

Helen, I'm really not satisfied about how the debugger looks with this patch. Can you should give suggestions about how to improve it ? I also changed the style editor sidemenu style.
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Attachment #8708807 - Flags: ui-review?(hholmes)
Attachment #8708807 - Flags: feedback?(bgrinstead)
(In reply to Tim Nguyen [:ntim] from comment #6)
> Helen, I'm really not satisfied about how the debugger looks with this
> patch. Can you should give suggestions about how to improve it ? I also
> changed the style editor sidemenu style.

It looks strange especially when breakpoints are shown into the debugger sidebar.
Comment on attachment 8708807 [details] [diff] [review]
WIP

Review of attachment 8708807 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/themes/splitview.css
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  .theme-dark {
> +  --sidemenu-selected-arrow: url(images/itemArrow-dark-ltr.svg);

Are these variables needed here?  They also seem to be included in widgets.css

::: devtools/client/themes/widgets.css
@@ +376,5 @@
>  /* SideMenuWidget groups */
>  
>  .side-menu-widget-group-title {
>    padding: 4px;
> +  font-weight: 500;

I'd prefer to generally keep the 'merge splitview and sidemenuwidget styling' and 'updating the current sidemenuwidget styling to match mockups' into separate patches as much as possible.

Related: How much work would it be to convert the Style Editor's SplitView instance into a SideMenuWidget?  Seems that's where we want to get to eventually and it might end up being easier than trying to support both at once.  If this worked we could delete current splitview js and css.
Attachment #8708807 - Flags: feedback?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #8)
> Comment on attachment 8708807 [details] [diff] [review]
> WIP
> 
> Review of attachment 8708807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/splitview.css
> @@ +3,5 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  .theme-dark {
> > +  --sidemenu-selected-arrow: url(images/itemArrow-dark-ltr.svg);
> 
> Are these variables needed here?  They also seem to be included in
> widgets.css
Whoops, Forgot to remove them.

> Related: How much work would it be to convert the Style Editor's SplitView
> instance into a SideMenuWidget?  Seems that's where we want to get to
> eventually and it might end up being easier than trying to support both at
> once.  If this worked we could delete current splitview js and css.
Not much, although, I've heard of plans about switching to common list widget across all tools, and plans to merge the Style Editor with the debugger to make a common source view. Which is why I think the work is not worth doing.
(In reply to Tim Nguyen [:ntim] from comment #9)
> > Related: How much work would it be to convert the Style Editor's SplitView
> > instance into a SideMenuWidget?  Seems that's where we want to get to
> > eventually and it might end up being easier than trying to support both at
> > once.  If this worked we could delete current splitview js and css.
> Not much, although, I've heard of plans about switching to common list
> widget across all tools, and plans to merge the Style Editor with the
> debugger to make a common source view. Which is why I think the work is not
> worth doing.

So switching to a common list widget across tools is going to happen I believe, and I'm sure we will have a fairly easy conversion for existing SideMenuWidgets but not for the splitview.  Regarding merging style editor and debugger, I think that's far enough down the road that we shouldn't worry about it (not even in design phase yet AFAIK).

Looking at the CSS needed to keep them both behaving similarly I'm thinking the right thing to do is to actually use a SideMenuWidget (either from within the split view or as a replacement to the split view).
Attached image comparison.png (obsolete) —
There are a few things I think you can do:

1. I would bump up the weight on the headers. I'd go with a bold or semibold (for SF UI I'd go with bold, but you'd have to fiddle for Tahoma.)
2. I'd keep the line separator. I think it's not enough contrast without it.
3. I'd increase the indentation more dramatically on the filenames. Less space, but more obvious.
4. I'd hang the checkboxes. (Checkboxes aren't technically punctuation to my knowledge, but I think they look better hanging, and hanging punctuation at least is considered good typography practice: https://en.wikipedia.org/wiki/Hanging_punctuation)
Attachment #8710010 - Flags: feedback+
Attachment #8708807 - Flags: ui-review?(hholmes) → feedback+
Attached patch Patch (obsolete) — Splinter Review
Purely CSS changes, no file deletions.

Also applied Helen's feedback (thanks Helen!).
Attachment #8634837 - Attachment is obsolete: true
Attachment #8634839 - Attachment is obsolete: true
Attachment #8708807 - Attachment is obsolete: true
Attachment #8712843 - Flags: review?(bgrinstead)
Blocks: 1242709
Comment on attachment 8712843 [details] [diff] [review]
Patch

Review of attachment 8712843 [details] [diff] [review]:
-----------------------------------------------------------------

Flagging for UI review.. I'm not sure I like the new designs compared with old but will defer to Helen's opinion on that and then I'll just review the code if she's happy with it.
Attachment #8712843 - Flags: ui-review?(hholmes)
Attached image bp-compare.png (obsolete) —
screenshots of various states on osx
Before I review: did something change with the selection states in the debugger, or is that the way the debugger currently acts if you select multiple things?

Tagging you bgrins since you uploaded the compare.png, sorry!
Flags: needinfo?(bgrinstead)
I actually also think it's worth tagging James in this conversation, since I know he's about to do a bunch of debugger UI refactoring that may negate the work being done in this bug, I'm not sure :/
Flags: needinfo?(jlong)
Thanks, I've been meaning to comment in here. This is closely related to my work in bug 1238012, which I'm just now figuring out how to make progress on. A few comments:

* What's the main reason for indenting the list items? If you look at a list where there isn't a source selected, it seems like the source names and bps will run together now. I like that the checkbox "indents" the bps right now and clearly breaks up the source names. We also lose a lot of space for the source name, which is already difficult to read. If we're trying to make this more like a nested tree, I think we should just wait until we can replace these with that (which should be a matter of months at the most).

* Like Helen said, what's up with multiple breakpoints being selected? You can't select multiple ones.

* I'm refactoring the right pane to be more like an accordion like Chrome, with several sections of info (breakpoints, call stack, watches, scope, etc). A lot of these sections will still use this widget for now, so it's worth at least updating the colors & bold fonts.

* You should also look at the Event Listeners tab in the right pane of the debugger and make sure that all makes sense with the new styles
Flags: needinfo?(jlong)
(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #15)
> Before I review: did something change with the selection states in the
> debugger, or is that the way the debugger currently acts if you select
> multiple things?
> 
> Tagging you bgrins since you uploaded the compare.png, sorry!

I guess you are talking about all of the breakpoints in a selected source sharing the blue selected background?  That's introduced in the patch and something I found confusing about it.

Given Comment 17 I'm thinking that we should keep this change limited to consolidating splitview and sidemenuwidget styling, possibly with a few tweaks if there is low-hanging improvement, but not have elements moving around too much.
Flags: needinfo?(bgrinstead)
Comment on attachment 8712843 [details] [diff] [review]
Patch

Review of attachment 8712843 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review for now - see Comment 17
Attachment #8712843 - Flags: review?(bgrinstead)
Comment on attachment 8712843 [details] [diff] [review]
Patch

Also clearing review, comment 17.
Attachment #8712843 - Flags: ui-review?(hholmes)
Attached patch Patch (obsolete) — Splinter Review
I think this addresses the UX concerns previously raised.
Attachment #8634841 - Attachment is obsolete: true
Attachment #8634842 - Attachment is obsolete: true
Attachment #8710010 - Attachment is obsolete: true
Attachment #8712843 - Attachment is obsolete: true
Attachment #8712911 - Attachment is obsolete: true
Attachment #8721688 - Flags: ui-review?(hholmes)
Attachment #8721688 - Flags: review?(bgrinstead)
Comment on attachment 8721688 [details] [diff] [review]
Patch

I'm not sure what I think about this pattern in general: http://cl.ly/0B0O0t2p3J23

I think it maybe looks strange because it creates one big long, l-ish sort of shape. Maybe that side line needs to just be thinner? Or we need a better way to distinguish headers.

On another note, the blues (subheader background, input checkbox background) don't match, which seems really obvious in these tight spaces.

I'd remove this highlighted border-bottom line at the bottom of the breakpoints section. It seems seems superfluous and again, it draws attention to the fact that we have multiple blues: http://cl.ly/2e0r1E3R0r2G
Attachment #8721688 - Flags: ui-review?(hholmes) → feedback+
Comment on attachment 8721688 [details] [diff] [review]
Patch

Review of attachment 8721688 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for splitview changes.  But seems like widgets.css might still need some UI work (i.e. comment 22).  Could we move the splitview styling patch into a separate bug and land it?  It doesn't seem to cause too many UI changes and it's a nice refactor so seems like we could proceed with that now, unless if there's a reason these two need to land together.

::: devtools/client/themes/performance.css
@@ +105,5 @@
>  
>  /* Sidebar & recording items */
>  
> +#recordings-list {
> +  border-inline-end: 1px solid var(--theme-splitter-color);

Why is this needed?

::: devtools/client/themes/splitview.css
@@ +33,4 @@
>    -moz-box-align: center;
>    outline: 0;
>    vertical-align: bottom;
> +  border-bottom: 1px solid rgba(128,128,128,0.15);

Please keep this hardcoded color as a variable at the top of the file
Attachment #8721688 - Flags: review?(bgrinstead)
(In reply to (Unavailable until 3-7) Brian Grinstead [:bgrins] from comment #23)
> Comment on attachment 8721688 [details] [diff] [review]
> Patch
> 
> Review of attachment 8721688 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ for splitview changes.  But seems like widgets.css might still need some
> UI work (i.e. comment 22).  Could we move the splitview styling patch into a
> separate bug and land it?  It doesn't seem to cause too many UI changes and
> it's a nice refactor so seems like we could proceed with that now, unless if
> there's a reason these two need to land together.
> 
> ::: devtools/client/themes/performance.css
> @@ +105,5 @@
> >  
> >  /* Sidebar & recording items */
> >  
> > +#recordings-list {
> > +  border-inline-end: 1px solid var(--theme-splitter-color);
> 
> Why is this needed?
The performance tool uses the SMW styling, and I'm removing the box-shadow that's the emulating the right border with this patch. The box-shadow was really just useful for the performance tool, since it's currently being hidden by the sibling splitter anyway.

> ::: devtools/client/themes/splitview.css
> @@ +33,4 @@
> >    -moz-box-align: center;
> >    outline: 0;
> >    vertical-align: bottom;
> > +  border-bottom: 1px solid rgba(128,128,128,0.15);
> 
> Please keep this hardcoded color as a variable at the top of the file
This hardcoded color doesn't change across themes, and it's only a generic gray color, so I don't think it's useful to keep it as a variable. The memory tool also uses it as a hardcoded color.
I can put it into a variable if you feel strongly.

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #22)
> Comment on attachment 8721688 [details] [diff] [review]
> Patch
> 
> I'm not sure what I think about this pattern in general:
> http://cl.ly/0B0O0t2p3J23
> 
> I think it maybe looks strange because it creates one big long, l-ish sort
> of shape. Maybe that side line needs to just be thinner? Or we need a better
> way to distinguish headers.
I'm going to reduce the line width to 2px (instead of 3px)

> On another note, the blues (subheader background, input checkbox background)
> don't match, which seems really obvious in these tight spaces.
We talked about this on IRC, and this is going to be changed in bug 1248621.

> I'd remove this highlighted border-bottom line at the bottom of the
> breakpoints section. It seems seems superfluous and again, it draws
> attention to the fact that we have multiple blues: http://cl.ly/2e0r1E3R0r2G
I will remove it in the next patch.
Attached patch PatchSplinter Review
Attachment #8721688 - Attachment is obsolete: true
Attachment #8723571 - Flags: review?(vporof)
Comment on attachment 8723571 [details] [diff] [review]
Patch

Review of attachment 8723571 [details] [diff] [review]:
-----------------------------------------------------------------

changes look good
Attachment #8723571 - Flags: review?(vporof) → review+
https://hg.mozilla.org/mozilla-central/rev/557f7ef0273a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Blocks: 1253981
Depends on: 1326630
Product: Firefox → DevTools
Component: Debugger → CSS and Themes
You need to log in before you can comment on or make changes to this bug.