Skip to content
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

Implement customizable cursor height #61714

Merged
merged 8 commits into from Jul 21, 2020
Merged

Implement customizable cursor height #61714

merged 8 commits into from Jul 21, 2020

Conversation

guidezpl
Copy link
Member

@guidezpl guidezpl commented Jul 17, 2020

Description

Cursor width can currently be customized. This PR adds the ability to customize cursor height for Cupertino and Material text fields (as well as EditableText and SelectableText).

Related Issues

Closes #26870
#61715

Tests

I added the following tests:

  • new golden test for EditableText cursor height
    editable_text_test 3
  • new golden test for Material text field cursor height
    material text_field_cursor_width_test 2
  • updated existing default property, debugFillProperties and plumbing tests

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. If not, delete the remainder of this section.

@fluttergithubbot fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jul 17, 2020
@skia-gold
Copy link

Gold has detected about 4 untriaged digest(s) on patchset 6.
View them at https://flutter-gold.skia.org/cl/github/61714

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM, just take a look at using pumpAndSettle as mentioned in a comment. Thanks for working on this!

@@ -625,6 +629,9 @@ class CupertinoTextField extends StatefulWidget {
properties.add(DiagnosticsProperty<bool>('expands', expands, defaultValue: false));
properties.add(IntProperty('maxLength', maxLength, defaultValue: null));
properties.add(FlagProperty('maxLengthEnforced', value: maxLengthEnforced, ifTrue: 'max length enforced'));
properties.add(DoubleProperty('cursorWidth', cursorWidth, defaultValue: 2.0));
properties.add(DoubleProperty('cursorHeight', cursorHeight, defaultValue: null));
properties.add(DiagnosticsProperty<Radius>('cursorRadius', cursorRadius, defaultValue: null));
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch adding these missing properties!

tester.state<EditableTextState>(textFinder).showToolbar();
await tester.pump();
await tester.pump(const Duration(milliseconds: 500));
await tester.pump(const Duration(milliseconds: 500));
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to use a pumpAndSettle here and below instead of the multiple pumps and durations, if you haven't tried it already.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could only replace one, I think it has to do with the cursor blinking and the timing needing to be right

await tester.tap(find.text('Paste'));
await tester.pump();
await tester.pump(const Duration(milliseconds: 500));
await tester.pump(const Duration(milliseconds: 500));
Copy link
Contributor

Choose a reason for hiding this comment

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

Strange, is the pump without a duration needed as well? And what if you combine the two with durations into one with Duration(seconds: 1)?

There are other tests that do this sort of thing, though not testing the cursor size.

Anyway I think the main thing is to just make sure it's clear to someone else reading this code why the pumps are there, so maybe just add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

It wasn't, I simplified this test and others doing the same thing, and added a comment

@fluttergithubbot fluttergithubbot merged commit 5d854f6 into flutter:master Jul 21, 2020
@guidezpl guidezpl deleted the text-field-cursor-height branch July 21, 2020 15:15
Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextField cursor height cannot be changed
5 participants