-
Notifications
You must be signed in to change notification settings - Fork 336
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
Add sorting functionality to flutter tables. #1738
Conversation
@@ -19,32 +19,30 @@ class CpuProfilerController { | |||
static CpuProfileData baseStateCpuProfileData = CpuProfileData.empty(); | |||
|
|||
/// Notifies that new cpu profile data is available. | |||
ValueListenable get dataNotifier => _dataNotifier; | |||
ValueListenable get data => _dataNotifier; |
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.
this refactor looks only half done. The privates still have _notifier in the name.
Also looks unrelated. Maybe move to a separate CL?
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.
This is what I did in timeline_controller.dart. I was actually in favor of leaving the ValueNotifier vars as *_notifier
since it is easy to tell which value is read only and which value is read-write. Either way, I'll revert back and do as a follow up CL.
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.
Great to see this. Done with a round of comments.
# Conflicts: # packages/devtools_app/lib/src/flutter/common_widgets.dart # packages/devtools_app/lib/src/ui/flutter/service_extension_widgets.dart
@@ -39,6 +39,8 @@ ThemeData _lightTheme() { | |||
} | |||
|
|||
const buttonMinWidth = 36.0; | |||
const defaultIconSize = 16.0; |
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.
fyi @devoncarew these constants might be relevant for some of your CLs as well.
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.
# Conflicts: # packages/devtools_app/lib/src/flutter/table.dart # packages/devtools_app/lib/src/logging/flutter/logging_screen.dart
Addresses part of https://github.com//issues/1657