Fix wrong position of TabBarIndicator when it's label size and has label padding #109605
Conversation
flutter-dashboard
bot
added
f: material design
xu-baolin
left a comment
xu-baolin
left a comment
There was a problem hiding this comment.
A more reasonable solution should be to achieve the purpose through the same value of labelPadding and indicatorPadding.
We only need to take into account the padding layout information of the tabs.
return Center(
heightFactor: 1.0,
child: KeyedSubtree(
key: _tabKeys[index],
child: Padding(
padding: adjustedPadding ?? widget.labelPadding ?? tabBarTheme.labelPadding ?? kTabLabelPadding,
child: widget.tabs[index],
),
),
);
Actual labelPadding is different for each tabs and base on adjustedPadding(it's changing frequently, i means the logic). RenderPadding is always here, findAncestorRenderObjectOfType method, In my opinion, it doesn't waste the performance. and it's not the same like _tabKeys, _initIndicatorPainter method is called before build. |
There was a problem hiding this comment.
Call findAncestorRenderObjectOfType to find the RenderPadding looks a little odd.
What will hanppen if the Padding widget remove in the future?
There was a problem hiding this comment.
- label padding is the padding outsize of label, if you put it into KeyedSubtree, the size from key is also changed, you can't clarify what is the TabBarIndicatorSize.label.
- if someone remove Padding, it will be a big breaking change for the customer, and test case will not be passed. and we are in nullsafety, 'EdgeInsetsGeometry? labelPadding ' should not be, if we already know it.
There was a problem hiding this comment.
Do we have a better way to get the label padding info?
There was a problem hiding this comment.
i have no better idea for now 😳
There was a problem hiding this comment.
Reaching out to the RenderObject does not look right. If this padding is controlled by the TabBar, we should pass it directly into the painter.
There was a problem hiding this comment.
tabLeft += (delta + insets.left)
There was a problem hiding this comment.
it seems that () is not necessary, 'Unnecessary parenthesis can be removed(unnecessary_parenthesis).'
xu-baolin
added
the
autosubmit
|
auto-submit
bot
removed
the
autosubmit
|
Validations Fail. |
|
@goderbauer Hi, Could you take a second review for this? |
There was a problem hiding this comment.
Reaching out to the RenderObject does not look right. If this padding is controlled by the TabBar, we should pass it directly into the painter.
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. Changes reported for pull request #109605 at sha 3dc39c12ef2b34ba6ed0bc56c1a42913c8a5a19a |
|
@goderbauer all tests are passed, please take a look if you have a time |
|
Sorry about the delay, but this PR slipped through the cracks. Could you pull in the latest master and resolve the merge conflicts? Thanks! |
3 participants
wrong position of TabBarIndicator when set indicatorSize to TabBarIndicatorSize.label and it has labelPadding.
Related issue:
#109604
#63700
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Add test to the case TabBarIndicatorSize.label and has labelPadding
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.