Repository navigation
Added CupertinoFormSection, CupertinoSplitFormRow, and CupertinoTextFormField - #70676
Merged
Merged
Conversation
flutter-dashboard
Bot
added
the
framework
DanielEdrisian
force-pushed
the
cupertino-form
branch
from
November 18, 2020 03:57
b3d9c50 to
7b95d06
Compare
flutter-dashboard
Bot
added
the
p: cupertino_ui
DanielEdrisian
changed the title
DanielEdrisian
marked this pull request as ready for review
November 18, 2020 04:14
|
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 Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
xster
reviewed
Nov 18, 2020
xster
left a comment
xster
left a comment
Member
There was a problem hiding this comment.
Sorry I have to finish later tonight. Some notes on the first file.
xster
reviewed
Nov 18, 2020
Member
There was a problem hiding this comment.
It's a bit of a hidden feature. But if there's verbatim doc in various places, you can search for usages of {@macro ... in various places
Contributor
Author
There was a problem hiding this comment.
Keeping this comment to see if my use of macro was correct. Will close once the documentation tool passes here.
xster
reviewed
Nov 19, 2020
xster
left a comment
xster
left a comment
Member
There was a problem hiding this comment.
Cool, overall approach looks good. I think we're ready for tests.
DanielEdrisian
force-pushed
the
cupertino-form
branch
from
November 21, 2020 03:56
1a7f636 to
ffa91c3
Compare
Contributor
There was a problem hiding this comment.
nit: is there a reason for making margins nullable, instead of using _kDefaultMargins as its default in the constructor?
Contributor
Author
There was a problem hiding this comment.
Here's my thought process:
CupertinoTextFormFieldRow also has a margins property that is passed onto CupertinoFormRow upon its composition. I would have to have the same private constant defined in two files if I wanted to have a non-nullable margins property, so i thought it was better to leave the default behavior as optional chaining instead. The alternative would be to create some class specifically to hold that default value, and then use that in both the text field row and the normal row. Is there a better way to go about this?
Member
|
you should be able to run |
xster
approved these changes
Nov 23, 2020
Contributor
Author
|
I made some changes to the CupertinoFormSection API @LongCatIsLooong @xster
|
xster
reviewed
Nov 25, 2020
xster
left a comment
xster
left a comment
Member
There was a problem hiding this comment.
Section and row still need tests but otherwise still LGTM
DanielEdrisian
force-pushed
the
cupertino-form
branch
from
November 26, 2020 01:19
d7dac09 to
cd7c964
Compare
Contributor
Author
|
Rebased on top of master |
Contributor
|
This pull request is not suitable for automatic merging in its current state.
|
9 of 13 tasks
DanielEdrisian
added a commit
to DanielEdrisian/flutter
that referenced
this pull request
Dec 3, 2020
…tinoTextFormField (flutter#70676)"
This was referenced Dec 5, 2020
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Added
CupertinoFormSection,CupertinoSplitFormRow, andCupertinoTextFormField.Demo:
https://github.com/DanielEdrisian/SharedStorage/blob/main/demo%20forms.mov
Related Issues
#69916
#26745
Tests
No tests.
Replace this with a list of the tests that you added as part of this PR. A change in behavior with no test covering it
will likely get reverted accidentally sooner or later. PRs must include tests for all changed/updated/fixed behaviors. See Test Coverage.
Checklist
Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.