Add banner message warnings and errors #1764
Conversation
| ), | ||
| ), | ||
| const SizedBox(width: denseSpacing), | ||
| CircularIconButton( |
There was a problem hiding this comment.
this icon looks quite large. is that intentional?
There was a problem hiding this comment.
@raison00 do you have a recommendation on how large this "dismiss" button should be? Right now it is set at 36px
There was a problem hiding this comment.
Google's minimum recommended touch target size is around 48px. WCAG suggests 44px or larger.
If the dismiss button had width/height of 24px, a smaller image imprint, the dismiss button should have additional padding to ensure the target size is within the 48px area.
Going below the 24px size is not recommended for the dismiss button.
This is a 24px with a 48px touch target for reference:
| textSpans: [ | ||
| const TextSpan( | ||
| text: | ||
| 'You are running your app in debug mode. Debug mode performance ' |
There was a problem hiding this comment.
nit: this is probably cleaner as a ''' literal. You wouldn't need to escape anything.
| child: Row( | ||
| children: <Widget>[ | ||
| Expanded( | ||
| child: Container( |
There was a problem hiding this comment.
@pq a lint should be able to catch that this Container widget can be removed.
There was a problem hiding this comment.
Indeed! avoid_unnecessary_containers should do just the trick.
| @@ -459,3 +459,43 @@ class CenteredMessage extends StatelessWidget { | |||
| ); | |||
| } | |||
| } | |||
|
|
|||
| class CircularIconButton extends StatelessWidget { | |||
There was a problem hiding this comment.
isn't there an existing material design close button we can use instead? this button seems a bit large and not that consistent with material design.
There was a problem hiding this comment.
open to suggestions. A button with text seemed too cluttered, a button with a rectangular outline seemed to clash with the rounded corners of the card., and a solo "X" icon seemed a little plain IMO. @raison00 thoughts?
| child: Padding( | ||
| padding: DevToolsScaffold.appPadding, | ||
| child: screen.build(context), | ||
| Padding( |
There was a problem hiding this comment.
@pq
Padding + Align should trigger a lint to use Container.
There was a problem hiding this comment.
Cool! I could use some context... Issue opened: https://github.com/dart-lang/linter/issues/2059. Maybe you could elaborate there when you have a moment? Thanks!
There was a problem hiding this comment.
When an Align widget has a child of Padding or vice versa, we should trigger a lint that suggests instead using Container so that both of those properties can be set in a single widget.
Container(
alignment: ...,
padding: ...,
);
packages/devtools_app/lib/src/performance/flutter/performance_screen.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart
Outdated
Show resolved
Hide resolved
|
High level I wonder if we can align the APIs for this with the Notifications.of and similar APIs. |
…Warning # Conflicts: # packages/devtools_app/lib/src/flutter/status_line.dart # packages/devtools_app/lib/src/flutter/theme.dart
…s into profileModeWarning # Conflicts: # packages/devtools_app/lib/src/flutter/status_line.dart
| data: this, | ||
| child: Column( | ||
| children: [ | ||
| ...controller?.messagesForScreen(widget.screen.type) ?? [], |
There was a problem hiding this comment.
refactoring this to
Column(
children: [
Column(
children: ValueListenableBuilder(controller?.messagesForScreen(widget.screen.type),
),
Expanded(...),
)will fix your issue with having to do a post frame callback and will make things work correctly if someone else uses the banner controller.
There was a problem hiding this comment.
doing this still results in the setState() or markNeedsBuild() called during build. error.
There was a problem hiding this comment.
darn. I guess the right mental model is you can't change any ValueListenable objects during build either.
| class BannerMessagesController implements DisposableController { | ||
| final _dismissedMessageIds = <String>{}; | ||
| class BannerMessagesController { | ||
| final _messages = <DevToolsScreenType, List<BannerMessage>>{}; |
There was a problem hiding this comment.
make the values in this map
ValueNotifier<List>
and be sure to call
notifyListeners on the ValueNotifier every time you change the content of the list.
Then users of this controller can be notified when the controller state changes instead of having to hope they call setState at the right time.
There was a problem hiding this comment.
the reason I didn't do this originally is because notifyListeners is protected
used notifyListeners anyway and suppressed the warnings
There was a problem hiding this comment.
sounds good. We could add a TODO to create our own sublcass that has a method to add an element to an existing List and call notifyListeners.
| ); | ||
| if (messageWithKey != null) { | ||
| removeMessage(messageWithKey); | ||
| } | ||
| } | ||
|
|
||
| bool isMessageDismissed(BannerMessage message) { |
There was a problem hiding this comment.
nit: you can probably make this message @Protected or private now.
There was a problem hiding this comment.
made visible for testing - here and below
| ); | ||
| if (messageWithKey != null) { | ||
| removeMessage(messageWithKey); | ||
| } | ||
| } | ||
|
|
||
| bool isMessageDismissed(BannerMessage message) { | ||
| return _dismissedMessageKeys.contains(message.key); | ||
| } | ||
|
|
||
| bool isMessageVisible(BannerMessage message) { |
There was a problem hiding this comment.
nit: you can probably make this message @Protected or private now.
| 'is not indicative of release performance.\n\nRelaunch your ' | ||
| 'application with the \'--profile\' argument, or ', | ||
| text: ''' | ||
| You are running your app in debug mode. Debug mode performance is not indicative of release performance. |
There was a problem hiding this comment.
does the extra '\n' before "You are running"
show up in how the text span is rendered?
| if (highProfileGranularityMessage != null) { | ||
| BannerMessages.of(context).remove(highProfileGranularityMessage); | ||
| } | ||
| BannerMessages.of(context).removeMessageByKey( |
Timeline and Performance pages:
Memory page:
Once a message is dismissed on a page, it will not be shown again on that page for a running DevTools instance. We should eventually add "Do not show this message again" functionality for warnings, and there is a TODO for that.
Fixes #1711