Skip to content

[TextInputLayout] Add baseline alignment support #56

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

[TextInputLayout] Add baseline alignment support #56

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be6b361
[TextInputLayout] Add baseline alignment support
SUPERCILEX Jan 20, 2018
d549adb
Merge remote-tracking branch 'upstream/master' into patch-2
SUPERCILEX Apr 13, 2018
d0b2d5d
Merge branch 'master' into patch-2
SUPERCILEX Jul 9, 2018
fba6413
Address style concerns
SUPERCILEX Sep 4, 2018
12ddd96
Address review comments
SUPERCILEX Sep 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/java/com/google/android/material/textfield/TextInputLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ public EditText getEditText() {
@Override
public int getBaseline() {
if (editText != null) {
return editText.getPaddingTop() + editText.getBaseline();
return editText.getBaseline() + getPaddingTop() + calculateLabelMarginTop();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking good, but boxBottomOffsetPx still needs to be accounted for. It should be added to the calculation only if boxBackgroundMode != BOX_BACKGROUND_NONE. You may want to split that logic out into a separate method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that's right. Here's a screenshot without the extra padding:

screenshot_20180904-153231

And here is with the extra padding:

screenshot_20180904-153407

BTW, here's my code:

int padding = getPaddingTop();
if (boxBackgroundMode != BOX_BACKGROUND_NONE) {
  padding += boxCollapsedPaddingTopPx;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The extra offset to account for is boxBottomOffsetPx, not boxCollapsedPaddingTopPx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, that will move the legacy widget down. The first screenshot was fine... what is there to fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't move the legacy widget down if the offset is only being added when boxBackgroundMode != BOX_BACKGROUND_NONE.

From what I can see in the first screenshot, the legacy text field is a little off. Can you add a screenshot that includes a TextView, EditText, and the TextInputLayout legacy and updated styles side by side?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, this looks great. I'll merge the change.

Thanks for contributing!

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the baseline does still appear to still be a little off:

image

Could you look around in TextInputLayout and see what could be causing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aw, dang! Can you share the XML you used so I can repro?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh never mind, that screenshot was using an earlier version of the code. I've verified that it works and I'll merge the changes.

Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Phew! 😌 Can't wait to see this finally go through!!! 😁👏🎉

} else {
return super.getBaseline();
}
Expand Down