-
Notifications
You must be signed in to change notification settings - Fork 338
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
Combine resume / pause buttons in the debugger controls #8858
base: master
Are you sure you want to change the base?
Conversation
@@ -37,6 +37,9 @@ TODO: Remove this section if there are not any general updates. | |||
|
|||
* Added a tooltip to describe the exception mode drop-down. - | |||
[#8849](https://github.com/flutter/devtools/pull/8849) | |||
* Either the 'pause' or the 'resume' buttons is displayed, | |||
never both. - | |||
[#4924](https://github.com/flutter/devtools/issues/4924) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Please link the PR instead of the issue that this fixes.
Uber nit on verbiage: entries should be phrased in the past tense (e.g. "Added XYZ" instead of "Add XYZ").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a nit on the release note entry to match the preferred style laid out in the release notes README.md
@@ -93,27 +93,27 @@ class _DebuggingControlsState extends State<DebuggingControls> | |||
final isSystemIsolate = controller.isSystemIsolate; | |||
return RoundedButtonGroup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we only have one item in items
, I don't think we need the RoundedButtonGroup
(since it's intended for a group of buttons). I think we probably want to use DevToolButton
here (though we might need to add a rounded
option which would match the UI of `RoundedButtonGroup). Adding @kenzieschmoll if she has other thoughts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 we can use DevToolsButton
instead. One clean up could be to change this method to return a ternary:
return isPaused ? DevToolsButton(...) : DevToolsButton(...);
;
Looks like there are also some failing tests that need to be updated: |
Thanks for the feedback. The test failure is probably more than I can do in a codespace, so if one of you wants to finish this off I'm fine with that. Otherwise I'll just close it without merging it. |
This fixes #4924 by showing only one button at any given time.