Skip to content

CupertinoTextField: added ability to change placeholder color#28001

Merged
xster merged 3 commits intoflutter:masterfrom
Nizarius:master
Mar 28, 2019
Merged

CupertinoTextField: added ability to change placeholder color#28001
xster merged 3 commits intoflutter:masterfrom
Nizarius:master

Conversation

@Nizarius
Copy link
Contributor

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

Googlers can find more info about SignCLA and this PR by following this link.

@Nizarius
Copy link
Contributor Author

Nizarius commented Feb 15, 2019

I signed it!

I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Googlers can find more info about SignCLA and this PR by following this link.

@Nizarius
Copy link
Contributor Author

I signed it

@googlebot
Copy link

CLAs look good, thanks!

Googlers can find more info about SignCLA and this PR by following this link.

@zoechi zoechi added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Feb 15, 2019
@Nizarius Nizarius changed the title add ability to change placeholder color Cupertino text field: add ability to change placeholder color Feb 15, 2019
@Nizarius Nizarius changed the title Cupertino text field: add ability to change placeholder color CupertinoTextField: add ability to change placeholder color Feb 15, 2019
@Nizarius Nizarius changed the title CupertinoTextField: add ability to change placeholder color CupertinoTextField: addded ability to change placeholder color Feb 15, 2019
@Nizarius Nizarius changed the title CupertinoTextField: addded ability to change placeholder color CupertinoTextField: added ability to change placeholder color Feb 15, 2019
@Nizarius
Copy link
Contributor Author

Should I change anything to resolve this PR?

@matthew-carroll
Copy link
Contributor

@Nizarius would you mind correcting the analysis issues?

@Nizarius
Copy link
Contributor Author

Nizarius commented Mar 2, 2019

@matthew-carroll yeah, of course.
By the way, it seems that it will be better to make not only placeholder color modifiable, but all the placeholder style at once. What do you think?
If you can make issue for that I can make pr to it. Because it's highly likely that developers want more than changing only placeholder color (for example, I want to modify font weight too and so on).

@matthew-carroll
Copy link
Contributor

@Nizarius yes, I think we should allow an arbitrary TextStyle in a property called placeholderStyle. Let's do that here. We wouldn't want to merge in color support and then immediately replace it. Can you update this PR accordingly, and ensure that all checks pass?

Also, would you mind adding some tests for this new behavior?

@Nizarius Nizarius force-pushed the master branch 4 times, most recently from 75ed614 to 10ae036 Compare March 6, 2019 06:51
@Nizarius
Copy link
Contributor Author

Nizarius commented Mar 6, 2019

@matthew-carroll I updated my PR to pass all the tests.
Frankly speaking, I'm new at Flutter and don't really understand what tests should I add for this new behavior.

@matthew-carroll
Copy link
Contributor

Thanks @Nizarius.

@xster let's discuss on Tuesday if we should carry this PR across the finish line.

@xster
Copy link
Member

xster commented Mar 11, 2019

Sure.

@Nizarius, adding tests does sound a bit overwhelming initially though if you take a look at https://github.com/flutter/flutter/blob/master/packages/flutter/test/cupertino/text_field_test.dart, there are already tons of examples that you can base new tests off of. Some even creates and asserts the styles of the placeholder text. I'd create a new test based off of that one to make sure custom placeholder styles are respected.

@Nizarius
Copy link
Contributor Author

@xster @matthew-carroll I fixed code according to your notes and also added test for placeholder's style.

@Nizarius
Copy link
Contributor Author

@xster @matthew-carroll hi guys. Is there anything I need to do?


/// The style to use for the placeholder text which overrides [style] property.
///
/// If null, placeholder's style will be the same as [style].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't true, is it? the placeholder should at least have a special color even if placeholderStyle isn't provided, right?

@xster
Copy link
Member

xster commented Mar 26, 2019

@Nizarius there's some trickiness with the whole TextStyle merge thing that isn't really easy to explain so we tweaked the doc in your branch a bit. Hope that's ok.

The rest of the PR looks good to us. We'll merge it in when our tree is green. Thanks for the contribution.

@xster xster merged commit ea343cc into flutter:master Mar 28, 2019
@Nizarius
Copy link
Contributor Author

Nizarius commented Mar 28, 2019

@xster @matthew-carroll thank you, guys. Hope Flutter will continue to evolve.

@xster
Copy link
Member

xster commented Mar 28, 2019

Thanks for your contribution

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CupertinoTextField unable to override the default placeholder color.

5 participants