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.

Stephen, I'm mainly concerned about how the debugger sidebar looks, especially the domain headers. What do you think ?

(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?

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.

(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.

(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).

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)

Purely CSS changes, no file deletions. Also applied Helen's feedback (thanks Helen!).

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.

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!

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 :/

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

(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.

Comment on attachment 8712843 [details] [diff] [review] Patch Review of attachment 8712843 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review for now - see Comment 17

Comment on attachment 8712843 [details] [diff] [review] Patch Also clearing review, comment 17.

I think this addresses the UX concerns previously raised.

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

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

(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.

Comment on attachment 8723571 [details] [diff] [review] Patch Review of attachment 8723571 [details] [diff] [review]: ----------------------------------------------------------------- changes look good