Skip to content

Move a few login-related helper classes to the utils library#6776

Merged
hypest merged 4 commits intodevelopfrom
feature/move-login-pieces-to-utils
Oct 25, 2017
Merged

Move a few login-related helper classes to the utils library#6776
hypest merged 4 commits intodevelopfrom
feature/move-login-pieces-to-utils

Conversation

@aforcier
Copy link
Copy Markdown
Contributor

@aforcier aforcier commented Oct 24, 2017

Moves ViewUtils and WPTextInputLayout to the utils library, where they can be used by the extracted login UI library (once we release a new artifact).

(WIP branch for the login extraction, for reference.)

There might be some other pieces in the future, but these are the most glaring ones so far (with all fragments except Prologue and Epilogue extracted).

cc @hypest

Background: Since the design support library was added to
the utils library, two cases of custom views that extend
android.widget classes were reported as errors by the linter,
prompting to instead extend AppCompat versions of those views.
@aforcier aforcier force-pushed the feature/move-login-pieces-to-utils branch from df9aa58 to 0370f89 Compare October 24, 2017 18:06
@hypest
Copy link
Copy Markdown
Contributor

hypest commented Oct 25, 2017

Hey @aforcier , nice to see this happening!

Quick q, are you planning to move WPLoginInputRow next perhaps? Should be fairly contained in itself I think. Asking since that's basically the only user of the WPTextInputLayout widget, and the reason the latter was created was just to work around a couple of issues of the 1st party TextInputLayout widget.

Copy link
Copy Markdown
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

Looks pretty much ready for merging @aforcier ! I've only left one comment about some dimens that might worth thinking about (if you haven't thought of already!) before merging. Thanks!

<dimen name="margin_text_input_layout_baseline">20dp</dimen>
<dimen name="margin_extra_extra_extra_large">92dp</dimen>
<dimen name="promo_indicator_bullet_size">4dp</dimen>
<dimen name="textinputlayout_baseline_correction">2dp</dimen>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we also move textinputlayout_correction_padding and textinputlayout_correction_margin_right. Those are part of the same set of "hacks" to fix issues of the TextInputLayout an pretty much are used in tandem. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, done in 1d6f478.

*
*/
public class AutoResizeTextView extends TextView {
public class AutoResizeTextView extends AppCompatTextView {
Copy link
Copy Markdown
Contributor

@hypest hypest Oct 25, 2017

Choose a reason for hiding this comment

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

Just wondering, can you share some background about this change? Is it some lint related warning or something else? Thanks! Scratch that, the individual commit message has the info I needed 👍 . Apologies and thanks!

@aforcier
Copy link
Copy Markdown
Contributor Author

are you planning to move WPLoginInputRow next perhaps? Should be fairly contained in itself I think. Asking since that's basically the only user of the WPTextInputLayout widget, and the reason the latter was created was just to work around a couple of issues of the 1st party TextInputLayout widget.

I felt that, since WPLoginInputRow relies on a layout using login styles, it made most sense to extract it to the login library, and not to utils. Here it is in the in-progress repo for the login flow: [link].

(WPTextInputLayout is also there, but will be dropped once this is merged and I push a new artifact for the utils lib.)

@hypest
Copy link
Copy Markdown
Contributor

hypest commented Oct 25, 2017

I felt that, since WPLoginInputRow relies on a layout using login styles, it made most sense to extract it to the login library, and not to utils. Here it is in the in-progress repo for the login flow: [link].

Aaah, I see, makes sense. All good 👍

@hypest
Copy link
Copy Markdown
Contributor

hypest commented Oct 25, 2017

LGTM!

@hypest hypest merged commit f1d9669 into develop Oct 25, 2017
@hypest hypest deleted the feature/move-login-pieces-to-utils branch October 25, 2017 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants