-
Notifications
You must be signed in to change notification settings - Fork 3k
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 rfw widgets #5661
Add rfw widgets #5661
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@text-exemption-reviewers" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
||
// In these widgets we make an effort to expose every single argument available. | ||
Map<String, LocalWidgetBuilder> get _coreWidgetsDefinitions => <String, LocalWidgetBuilder>{ |
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.
can you keep the original formatting? it's much, much easier to review changes with the old formatting than with what dart format does.
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.
Reverted. Should I format it in another pr or it's ok to just keep it what it is?
This will need tests. In particular, this package requires 100% test coverage. |
Done! I updated the golden files as well. It seems disabled now though. |
final length = source.length(['items']); | ||
final dropdownMenuItems = List<DropdownMenuItem<Object>>.generate( | ||
length, | ||
(i) => DropdownMenuItem<Object>( |
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.
might be clearer if i
was called index
or some such
also, missing type ((int index)
)
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.
Done!
/// * [DropdownButton] takes a list of items object with the key of params | ||
/// names and value of the param values to represent [DropdownMenuItem]. |
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.
I'm not sure exactly what this is trying to say but it doesn't seem to match the code — it seems to be a list of objects, and in each object there's zero or more of onTap
, value
, enabled
, alignment
, and child
? (Not sure which are required and which are optional, but we should document that too.)
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.
Updated the comment.
length, | ||
(i) => DropdownMenuItem<Object>( | ||
onTap: source.voidHandler(['items', i, 'onTap']), | ||
value: source.v<String>(['items', i, '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.
does it matter that this has to be a string, given that the "current" value can be a string, int, double, or bool?
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.
You're right. This can be any given type. Fixed.
@@ -135,15 +157,28 @@ void main() { | |||
await expectLater( | |||
find.byType(RemoteWidget), | |||
matchesGoldenFile('goldens/material_test.scaffold.png'), | |||
skip: !runGoldens, | |||
// skip: !runGoldens, |
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.
don't forget to uncomment this out
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.
Oops. Uncommented.
await expectLater( | ||
find.byType(MaterialApp), | ||
matchesGoldenFile('goldens/material_test.dropdown.png'), | ||
// skip: !runGoldens, |
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.
ditto
Added some comments, but overall looks pretty good! |
packages/rfw/CHANGELOG.md
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
* Updates minimum supported SDK version to Flutter 3.10/Dart 3.0. | |||
* Fixes lint warnings. | |||
* Add one core widget and one material widget. |
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.
you should say which ones :-)
remember the old trick when writing documentation: what question is someone trying to answer when reading your prose? what questions will they have after reading your prose?
'ClipRRect': (BuildContext context, DataSource source) { | ||
return ClipRRect( | ||
borderRadius: ArgumentDecoders.borderRadius(source, ['borderRadius']) ?? BorderRadius.zero, | ||
// clipper, |
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.
looks like the convention for skipped parameters in this file is to give the type as well (see line 441)
The goldens are Linux-only. |
Friendly ping:) |
@peixinli Sorry for not being clear. I was waiting for the test failures to be resolved before doing a final review pass. (No point giving an LGTM before the code is final, and the code is unlikely to be final if it's still failing tests. :-) ) |
Done! Sorry I'm still learning the process. |
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.
flutter/packages@8fbdf65...21b5abb 2024-01-24 [email protected] Add rfw widgets (flutter/packages#5661) 2024-01-24 [email protected] [camera] add heif support image iOS (flutter/packages#4586) 2024-01-24 [email protected] [webview_flutter_android] Add javascript panel interface for android (flutter/packages#5796) 2024-01-24 [email protected] [camerax] Wrap Android classes/methods required for implementing setting focus & exposure points and offset (flutter/packages#5659) 2024-01-24 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.2.0 to 4.3.0 (flutter/packages#5967) 2024-01-24 [email protected] Roll Flutter from 5b673c2 to 19b06f4 (21 revisions) (flutter/packages#5968) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Add two rfw widgets(ClipRRect and DropdownButton) and `flutter format` the widget files. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use `dart format`.) - [x] I signed the [CLA]. - [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. `[shared_preferences]` - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes]. - [x] I updated `CHANGELOG.md` to add a description of the change, [following repository CHANGELOG style]. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [relevant style guides]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat [pub versioning philosophy]: https://dart.dev/tools/pub/versioning [exempt from version changes]: https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#version-and-changelog-updates [following repository CHANGELOG style]: https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changelog-style [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
Add two rfw widgets(ClipRRect and DropdownButton) and
flutter format
the widget files.Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.