Repository navigation
Add minimum height and width box constraints to input decoration icons - #18480
Conversation
| data: new IconThemeData( | ||
| color: iconColor, | ||
| size: iconSize, | ||
| Widget prefixIcon; |
There was a problem hiding this comment.
I believe that the original syntax, 'final ... foo == null ? null : ..." is used consistently in this file.
|
Constraining the size of the prefix and suffix widgets seems reasonable to me if the overall layout isn't disturbed given text or icon values for the prefix/suffix. |
|
@HansMuller The layout is slightly disturbed, see the screenshot I added on the edited original message. I could replace the constrained box with padding based on the icon size, and then remove the padding between the prefix/suffix icons and the border. I am not sure how well this will work if we allow arbitrary widgets here |
|
The demo just has the one suffix icon (password field) and it does appear to be noticeably adjusted inwards. On the other hand the MD spec now appears to accommodate a 48x48 icon:
I think constraining the minimum sizes of the prefix and suffix widgets to 48x48 is OK. |
|
Update: I've made the prefix/suffix icons responsible for padding by removing the L/R padding if they are defined. Their touch area is lined up exactly with the end of the text field, and there are 12 px between the start of the icon and the edge of the text field
|
|
This looks good! Please add tests that verify the layout of prefix and suffix icons and update the doc to warn users that the prefix and suffix icon widgets will be constrained to be at least 48x48 and will be left or right justified. And the existing note/example about padding might not be useful anymore. |
|
Updated doc comments and added test case |
| ), | ||
| ); | ||
|
|
||
| // Overall height for this InputDecorator is 48dps: |
There was a problem hiding this comment.
Something is wrong here. The listed heights add up to 40 and the test checks for an overall height of 40, but the comment (and the point of this PR) say we're expecting an overall height of 48.
There was a problem hiding this comment.
Looking into it... might be drawing outside of bounds If I didn't correctly update something in the render object
There was a problem hiding this comment.
Confirmed locally that without a border the icons are drawn outside of bounds
There was a problem hiding this comment.
_RederDecoration.layout() should be taking the heights of the prefix and suffix icons into account when it computes containerHeight. It doesn't appear to be doing that - which would be an existing bug.
There was a problem hiding this comment.
Not to put too fine a point on it, but. I think this:
double containerHeight = contentPadding.top
+ aboveBaseline
+ belowBaseline
+ contentPadding.bottom;
Should be:
double containerHeight = contentPadding.top
math.max(aboveBaseline + belowBaseline, math.max(_boxSize(prefixIcon.height), _boxSize(suffixIcon.height))),
+ contentPadding.bottom;
There was a problem hiding this comment.
needs to take math.max over both prefix/suffix icons vs the baselines, since the don't have extra padding.
|
On the method below, adding prefixIcon and suffixIcon cause the min intrinsic height to be calculated correctly as 48px. However the RenderObject is also given tight constraints which might be incorrect. |
| + belowBaseline | ||
| + contentPadding.bottom; | ||
| + contentPadding.bottom, | ||
| math.max( _boxSize(suffixIcon).height, _boxSize(prefixIcon).height)); |
There was a problem hiding this comment.
This doesn't look right to me.
There was a problem hiding this comment.
Updated to formula discussed in chat
| return contentPadding.top | ||
| + (label == null ? 0.0 : decoration.floatingLabelHeight) | ||
| + _lineHeight(width, <RenderBox>[prefix, input, suffix]) | ||
| + _lineHeight(width, <RenderBox>[prefix, prefixIcon, input, suffix]) |
There was a problem hiding this comment.
This doesn't look right either. We should talk.
Update comment
|
LGTM This PR also fixes a layout bug that had gone unnoticed because no test. NICE |
* master: Add minimum height and width box constraints to input decoration icons (flutter#18480) Fix aot_snapshot_size_* benchmarks (flutter#18520) Revert "Remove race conditions involving finding available ports (flutter#18488)" (flutter#18521)
3 participants
I've added a set of box constraints around both the prefix and suffix input decorations. This does change the visual appearance of the input field, since they appear to have more spacing between them and the outer border.
Fixes #16672
@HansMuller , is this a reasonable approach to take? I know that there is an issue filled currently to support arbitrary widgets in these decorations. I worry that if I try to remove some of the padding and we add support for that, a larger widget would end up touching the border of the text field.