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

Extend InputDecoration with prefix/suffix padding #19514

Merged
merged 3 commits into from
Aug 3, 2018
Merged

Extend InputDecoration with prefix/suffix padding #19514

merged 3 commits into from
Aug 3, 2018

Conversation

christian-muertz
Copy link
Contributor

@christian-muertz christian-muertz commented Jul 18, 2018

Allows to make some space between the suffix/prefix text and the text entered into the text field.
(fixes #19460 @HansMuller)

@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

@christian-muertz
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@HansMuller
Copy link
Contributor

This looks reasonable however It might be simpler to just expose prefix/suffix widgets as an alternative to prefixText and suffixText. The input decorator internals (see _RenderDecorator etc) are already set up that way. Supporting widget values is more consistent with Flutter's overall approach and it scales better than adding more properties like this.

@christian-muertz
Copy link
Contributor Author

Yep thought about this too, but if we remove prefixText and suffixText it might break some apps.

What about exposing prefix and suffix too and only use prefixText and suffixText when no prefix/suffix is set?

@christian-muertz
Copy link
Contributor Author

What about replacing prefixIcon, prefixText, ... with only prefix? Having so many options for specifying the prefix is confusing.

@HansMuller
Copy link
Contributor

What about exposing prefix and suffix too and only use prefixText and suffixText when no prefix/suffix is set?

Sound good. We should assert if both attributes, say prefix and prefixText, are specified.

We can't get rid of prefixText and suffixText because backwards compatibility. Likewise, we can't replace prefixText and prefixIcon with just prefix. Ofcourse if we were starting over, that would make sense.

@christian-muertz
Copy link
Contributor Author

christian-muertz commented Jul 18, 2018

Alright, will work on it tomorrow

@christian-muertz
Copy link
Contributor Author

Ok replaced the padding parameters with prefix and suffix.

We should assert if both attributes, say prefix and prefixText, are specified.

Yep, when both prefix and prefixText is declared an assertion error is thrown. Same applies for suffix and suffixText.

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

This is looking pretty good. It will need tests.

final Widget prefix = decoration.prefixText == null ? null :
new AnimatedOpacity(
final Widget prefix = decoration.prefix == null && decoration.prefixText == null ? null
: new AnimatedOpacity(
Copy link
Contributor

Choose a reason for hiding this comment

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

The 2-space indentation got clobbered here and below.

@@ -1833,6 +1833,10 @@ class InputDecoration {
/// no border is drawn.
///
/// The [enabled] argument must not be null.
///
/// Only [prefix] or [prefixText] can be declared.
Copy link
Contributor

Choose a reason for hiding this comment

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

Only one of [prefix] and [prefixText] can be specified.

@@ -1833,6 +1833,10 @@ class InputDecoration {
/// no border is drawn.
///
/// The [enabled] argument must not be null.
///
/// Only [prefix] or [prefixText] can be declared.
/// Declaring both ends with an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally don't include statements like this in the docs.

final Widget prefix = decoration.prefixText == null ? null :
new AnimatedOpacity(
final Widget prefix = decoration.prefix == null && decoration.prefixText == null ? null
: new AnimatedOpacity(
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to honor the prefix textStyle here. I'd suggest defining a trivial private widget like _AffixText that deals with suffix and prefix and their AnimatedOpacity:

new _AffixText(
  text: prefixText, 
  chlid: prefix, 
  style: decoration.prefixStyle ?? hintStyle,
  labelIsFloating: widget._labelIsFloating,
)

Where _AffixText either builds a DefaultTextStyle that contains child or a new Text widget with the specified style.

final Widget suffix = decoration.suffixText == null ? null :
new AnimatedOpacity(
final Widget suffix = decoration.suffix == null && decoration.suffixText == null ? null
: new AnimatedOpacity(
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use _AffixText here as well.

@@ -2258,34 +2291,40 @@ class InputDecoration {

@override
int get hashCode {
// split into multiple hashValues calls
// because the hashValues function only takes up to 20 fields...
return hashValues(
Copy link
Contributor

@HansMuller HansMuller Jul 19, 2018

Choose a reason for hiding this comment

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

This statement can be left as it is - two calls to hashValues(). Just move a few of the parameters to the second call up into the first parameter list.

@@ -2035,6 +2046,14 @@ class InputDecoration {
/// See [Icon], [ImageIcon].
final Widget prefixIcon;

/// Optional widget to place on the line before the input.
/// Can be for example used to add some padding to the [prefixText] or to
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be for example => Can be

@@ -2074,6 +2093,14 @@ class InputDecoration {
/// See [Icon], [ImageIcon].
final Widget suffixIcon;

/// Optional widget to place on the line after the input.
/// Can be for example used to add some padding to the [suffixText] or to
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be for example => Can be

@@ -2035,6 +2046,14 @@ class InputDecoration {
/// See [Icon], [ImageIcon].
final Widget prefixIcon;

/// Optional widget to place on the line before the input.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth explaining that the prefix widget's baseline will be lined up with the input baseline.

@@ -2074,6 +2093,14 @@ class InputDecoration {
/// See [Icon], [ImageIcon].
final Widget suffixIcon;

/// Optional widget to place on the line after the input.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth explaining that the suffix widget's baseline will be lined up with the input's baseline.

@christian-muertz
Copy link
Contributor Author

Thanks! I really appreciate your advice. I am struggling a bit with my english but i will try my best in future commits 😅

@christian-muertz
Copy link
Contributor Author

christian-muertz commented Jul 19, 2018

I am not that familiar with testing flutter widgets but I will try it.

new _AffixText(
labelIsFloating: widget._labelIsFloating,
text: decoration.prefixText,
style: decoration.prefixStyle,
Copy link
Contributor

Choose a reason for hiding this comment

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

style shoudl be decoration.prefixStyle ?? hintStyle, as before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it

new _AffixText(
labelIsFloating: widget._labelIsFloating,
text: decoration.suffixText,
style: decoration.suffixStyle,
Copy link
Contributor

Choose a reason for hiding this comment

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

style should be decoration.suffixStyle ?? hintStyle, as before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ups fixed it

@christian-muertz
Copy link
Contributor Author

Is it ok if i rebase and squash?

@HansMuller
Copy link
Contributor

Is it ok if i rebase and squash?
Sure

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

The test looks good. I will land this PR once you've finished with the squash/rebase.

LGTM

@christian-muertz
Copy link
Contributor Author

Here we go

@HansMuller
Copy link
Contributor

Can you fix up the analyzer errors? Const is no longer needed in subexpressions of a const expression.

@christian-muertz
Copy link
Contributor Author

christian-muertz commented Aug 3, 2018

Yep all checks pass :D

@HansMuller HansMuller merged commit ad16374 into flutter:master Aug 3, 2018
@christian-muertz christian-muertz deleted the input_decoration_prefix branch August 3, 2018 16:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InputDecoration prefixPadding
3 participants