Skip to content

TextField's hintText should support TextDirection. - #69534

Merged
dkwingsmt merged 6 commits into
flutter:masterfrom
xclud:hint-text-direction
Dec 26, 2020
Merged

dkwingsmt merged 6 commits into
flutter:masterfrom
xclud:hint-text-direction

Conversation

@xclud

@xclud xclud commented Nov 1, 2020 •

Copy link
Copy Markdown
Contributor

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.

hint-text-direction

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:

import 'package:flutter/material.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(

        primarySwatch: Colors.blue,
      ),
      home: MyHomePage(title: 'Hint Text Direction'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  MyHomePage({Key key, this.title}) : super(key: key);

  final String title;

  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
      body: Directionality(
        textDirection: TextDirection.rtl,
        child: Padding(
          padding: const EdgeInsets.all(8.0),
          child: Column(
            children: <Widget>[
              TextFormField(
                textDirection: TextDirection.ltr,
                decoration: InputDecoration(
                  border: OutlineInputBorder(),
                  labelText: 'آدرس ایمیل',
                  hintText: '[email protected]',
                ),
              ),
              SizedBox(height: 8),
              TextFormField(
                textDirection: TextDirection.ltr,
                decoration: InputDecoration(
                    border: OutlineInputBorder(),
                    labelText: 'آدرس ایمیل',
                    hintText: '[email protected]',
                    hintTextDirection: TextDirection.ltr),
              ),
            ],
          ),
        ),
      ),
    );
  }
}

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change.

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.
@google-cla google-cla Bot added the cla: yes label Nov 1, 2020
@goderbauer goderbauer added p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Nov 2, 2020
@flutter-dashboard

Copy link
Copy Markdown

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
HansMuller requested a review from dkwingsmt November 6, 2020 00:53
@xclud

xclud commented Nov 18, 2020

Copy link
Copy Markdown
Contributor Author

@HansMuller @dkwingsmt any progress?

@dkwingsmt dkwingsmt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@xclud

xclud commented Nov 20, 2020

Copy link
Copy Markdown
Contributor Author

@dkwingsmt I've added the tests. Would you please review again.

@xclud
xclud requested a review from dkwingsmt November 22, 2020 18:57
@xclud

xclud commented Dec 4, 2020

Copy link
Copy Markdown
Contributor Author

@dkwingsmt Can you please review again?

@xclud

xclud commented Dec 9, 2020

Copy link
Copy Markdown
Contributor Author

@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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The entire body should indent 2 fewer spaces.

Suggested change
await tester.pumpWidget(MaterialApp(
await tester.pumpWidget(MaterialApp(

final hintTextFinder = find.text('Some Hint');

Text hintText = tester.firstWidget(hintTextFinder);
expect(hintText.textDirection, TextDirection.ltr);

@dkwingsmt dkwingsmt Dec 9, 2020 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not Directionality.of for this test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@dkwingsmt

Copy link
Copy Markdown
Contributor

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 flutter analyze --dartdocs --flutter-repo locally.)

After that we're good to merge!

@xclud
xclud requested a review from dkwingsmt December 11, 2020 07:53
@dkwingsmt

Copy link
Copy Markdown
Contributor

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.)

@xclud

xclud commented Dec 25, 2020

Copy link
Copy Markdown
Contributor Author

Hi @dkwingsmt. I did as you requested. I hope this will be merged. Merry Christmas and have great holidays.

@dkwingsmt
dkwingsmt merged commit 8f5d037 into flutter:master Dec 26, 2020
@xclud
xclud deleted the hint-text-direction branch January 6, 2021 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants