Repository navigation
TextField's hintText should support TextDirection. - #69534
Conversation
There are many cases for RTL languages that the TextField's label is RTL but the input direction is LTR (e.g. email address). Therefore we may need to change the directionality of the hintText.
|
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. |
|
@HansMuller @dkwingsmt any progress? |
dkwingsmt
left a comment
There was a problem hiding this comment.
Thanks for the PR! Overall looking pretty good, although we do want to use a test to prevent regression. There are at least 2 things I think worth testing:
- The parameter correctly overwrites the hint text's text direction;
- When it's null, the text direction is correctly inherited from its parent.
You can check out how other tests verify text direction, but I think the easiest way is to use golden tests (i.e. comparing screenshots), which you can find here
|
@dkwingsmt I've added the tests. Would you please review again. |
|
@dkwingsmt Can you please review again? |
|
@HansMuller It seems this PR is stuck without any progress. Would you please reveiw? |
| }); | ||
|
|
||
| testWidgets('TextFormField respects hintTextDirection', (WidgetTester tester) async { | ||
| await tester.pumpWidget(MaterialApp( |
There was a problem hiding this comment.
The entire body should indent 2 fewer spaces.
| await tester.pumpWidget(MaterialApp( | |
| await tester.pumpWidget(MaterialApp( |
| final hintTextFinder = find.text('Some Hint'); | ||
|
|
||
| Text hintText = tester.firstWidget(hintTextFinder); | ||
| expect(hintText.textDirection, TextDirection.ltr); |
There was a problem hiding this comment.
Why not Directionality.of for this test?
There was a problem hiding this comment.
Because Directionality.of will be null here. Then it will default to App/OS's default directionality. This is just my idea, I will change it as you say.
|
The test is looking pretty solid. There are still a few linting errors, pointed out by the "analyze-linux" check. Please make sure they are fixed. (You can also run After that we're good to merge! |
|
Hello xclud, can you try to update with the latest master and push again? It seems that there's some new API that the customer test recently adopted and can not be run on the revision this PR is based on. Everything else looks perfect. (My apology for the slow response due to the holiday season.) |
|
Hi @dkwingsmt. I did as you requested. I hope this will be merged. Merry Christmas and have great holidays. |
Description
There are many cases for RTL languages that the TextField's label is RTL but the input direction is LTR (e.g. email address). Therefore we may need to change the directionality of the hintText.
Related Issues
I am not aware of.
Tests
I didn't write any tests for this. It's very straight forward PR. But I've wrote and example. See screenshot above and below code:
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.