-
Notifications
You must be signed in to change notification settings - Fork 13
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
Refactor output tabs, test running and dark mode in Embedded playground #417
Conversation
} | ||
|
||
Future<void> _openJavaMinimalWordCount(WidgetTester wt) async { | ||
await wt.navigateAndSettle(_url); |
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.
test without expect?
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 not a test but a helper method like we had in standalone_change_example_sdk_run_test.dart
It is extracted so that its name gives a hint of the example name.
_result = const RunCodeResult( | ||
status: RunCodeStatus.compileError, | ||
errorMessage: kPipelineOptionsParseError, | ||
_setResult( |
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.
turn it to a setter?
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.
It does not have a corresponding private getter.
* limitations under the License. | ||
*/ | ||
|
||
import 'package:flutter/widgets.dart'; |
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.
import 'package:flutter/foundation.dart';
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.
+
} | ||
|
||
/// Marks [key] as read. | ||
void clearKey(K key) { |
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.
markRead
?
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.
+
} | ||
|
||
/// Marks all keys as read. | ||
void clear() { |
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.
markReadAll
?
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.
+
final _unreadKeys = <K>{}; | ||
|
||
/// Marks [key] as unread if [value] differs from the last call. | ||
void setValue(K key, dynamic value) { |
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.
addUnread
?
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.
No, because we check if the value is new, and it's not a caller's job to know if a call results in anything, this controller is like a simple ValueNotifier it them. We can even rename this to MapValueNotifier
or something like this later.
} | ||
|
||
void _updateGraphPainterIfNeed() { | ||
final graph = widget.playgroundController.codeRunner.result?.graph ?? ''; |
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.
may be make getter?
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 long chain is only used once.
|
||
@override | ||
Widget build(BuildContext context) { | ||
final graphPainter = _graphPainter; |
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.
unnecessary?
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.
It eliminates !
s below.
|
||
@override | ||
Widget build(BuildContext context) { | ||
final ext = Theme.of(context).extension<BeamThemeExtension>()!; |
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.
theme
?
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.
theme
is for ThemeData
objects, theme extensions are different.
We have ext
for this in 7 other locations, this got kind of standard.
}); | ||
|
||
final bool padding; |
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.
hasPadding
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.
+
|
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.