Improve the VS Code sidebar UI #6380
Conversation
DanTup
requested review from
bkonyi and
kenzieschmoll
and removed request for
a team
Can we just use the Dart Icon? @InMatrix @Nancyhu2023 should we create an icon specific to DevTools (like the Dart icon with a wrench or something?) |
| return TableRow( | ||
| children: [ | ||
| Text( | ||
| '${session.name} (${session.flutterMode})', |
There was a problem hiding this comment.
instead of showing null as a literal, can we just show 'Session starting...' until we know we have non null values for both of these?
There was a problem hiding this comment.
I dropped the mode if it's null. I think if we want to show explicitly that it's starting, we should tweak the API to make a more explicit status rather than using this field to infer it (I think they're equivalent but it feels like a slightly weird assumption).
| children: [ | ||
| Text( | ||
| '${session.name} (${session.flutterMode})', | ||
| style: Theme.of(context).textTheme.titleSmall, |
There was a problem hiding this comment.
what about Theme.of(context).regularTextStyle here instead?
There was a problem hiding this comment.
Done! Although I'm not actually sure what the difference is between that and just not having it. When I add that line and/or comment it out, the spacing of some of the letters changes very slightly when I hot reload, but only for one session and not the other. Seems almost like some rounding issue:
textstyle.mp4
| // TODO(dantup): Ensure the order matches the DevTools tab bar (if | ||
| // possible, share this order). |
There was a problem hiding this comment.
Checkout the ScreenMetaData enum in screen.dart. We can try to match the order there. If we do this, we should also add a test to visible_screens_test.dart that ensures that the order of DevTools screens matches the order specified in the enum.
| return Directionality( | ||
| textDirection: reversedDirection, |
There was a problem hiding this comment.
why do we need this directionality widget?
There was a problem hiding this comment.
It's to have the menu anchor on the right and expand left (and also to put the icons on the right). I've added a comment to make this clearer.
| style: | ||
| TextStyle(color: isSelected ? colorScheme.onPrimary : null), |
There was a problem hiding this comment.
can we do Theme.of(context).regularTextStyle.copyWith(color: ...)?
There was a problem hiding this comment.
Done! I also applied regularTextStyle to the TextButton because it was making the text bold by default which didn't match the debug sessions.
| textDirection: reversedDirection, | ||
| // Reverse the direction so the menu is anchored on the far side and | ||
| // expands in the opposite direction with the icons on the right. | ||
| textDirection: normalDirection, |
There was a problem hiding this comment.
if we want this revered, shouldn't this be reversedDirection?
There was a problem hiding this comment.
Ug sorry, this was an accidental change after I took the screenshot above. Fixed.
| for (final device in devices) | ||
| _createDeviceRow( | ||
| colorScheme, | ||
| Theme.of(context), |
There was a problem hiding this comment.
store theme in a var at the top of the build method so we don't have to call Theme.of(context) more than once
How about the construction icon from Google Symbols and Icons? https://fonts.gstatic.com/s/i/short-term/release/googlesymbols/construction/default/24px.svg. 
|
|
I've switched to the construction icon for now, but can easily change if we find something better (or DevTools gets a specific icon). |
3 participants
Some tweaks to the sidebar UI. Currently looks like this:
sidebar2.mp4
Things that still need doing:
(null)for mode while starting@kenzieschmoll @anderdobo