-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Added main ToS/PP #1300
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
Added main ToS/PP #1300
Conversation
@SUPERCILEX FYI: @lsirac is a new member of the Firebase Auth team and is going to be helping out on FirebaseUI, changing the ToS/PP flow to be compliant is his first task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good! Some comments,
@@ -175,4 +181,14 @@ private void validateAndProceed() { | |||
mHandler.fetchProvider(email); | |||
} | |||
} | |||
|
|||
private void setUpTermsOfService(View view) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be clearer if it accepted a TextView
argument and the TextView
was located by onViewCreated
@@ -28,6 +28,14 @@ | |||
|
|||
</android.support.design.widget.TextInputLayout> | |||
|
|||
<TextView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add something like tools:text="Terms and stuff"
it will be visible in the design editor, which makes things easier.
@@ -28,6 +28,14 @@ | |||
|
|||
</android.support.design.widget.TextInputLayout> | |||
|
|||
<TextView | |||
android:id="@+id/email_tos_and_pp_text" | |||
style="@style/FirebaseUI.Text.BodyText" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should get someone (is jnease still on the team?) to pick a better font size / color for this. It's too dark and too big IMO but I am pretty design challenged.
Also I think we may want this below the NEXT
button but, again, I'd like a second opinion.
|
||
private void setUpTermsOfService(View view) { | ||
List<AuthUI.IdpConfig> providers = getFlowParams().providerInfo; | ||
if (providers.size() == 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a convenience method to FlowParams
that does this logic for you since we'll need it in a few places? Something like flowParams.isSingleProviderFlow()
?
@@ -173,6 +169,18 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceStat | |||
} | |||
} | |||
|
|||
private @StringRes int getTermsStringResource() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is unused. Did you mean to use or remove it?
@@ -55,8 +74,9 @@ private void setPreamble(TextView textView) { | |||
textView.setText(mBuilder); | |||
} | |||
|
|||
private void setupCreateAccountPreamble() { | |||
String withTargets = getPreambleStringWithTargets(); | |||
private void setupPreamble(@StringRes int textViewText) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: don't really like this method name because setupPreamble
and setPreamble
are so similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
auth/src/main/res/values/strings.xml
Outdated
@@ -5,6 +5,8 @@ | |||
<string name="fui_default_toolbar_title" translatable="false">@string/app_name</string> | |||
<string name="fui_progress_dialog_loading" translation_description="Loading text in dialog">Loading…</string> | |||
<string name="fui_sign_in_default" translation_description="Button text to sign in">Sign in</string> | |||
<string name="fui_tos_and_pp" translation_description="Global ToS and Privacy policy message">By continuing, you are indicating that you accept our <xliff:g example="https://google.com/terms" id="tos" translation_description="">%1$s</xliff:g> and <xliff:g example="https://google.com/privacy" id="pp" translation_description="">%2$s</xliff:g>.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think this text is final let me know and I will ship them off for translation.
…samsterns feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome sauce, hi @lsirac 👋! Some style comments, questions about edge cases, and general feedback. Enjoy and welcome to the team! 🤗 (That was supposed to be a hug emoji... 😕😂)
PS: do you mind sharing your name? (I'm guessing it's not "Lsirac" 😉)
@@ -128,4 +131,24 @@ public FlowParameters createFromParcel(Parcel in) { | |||
return new FlowParameters[size]; | |||
} | |||
}; | |||
|
|||
public boolean isSingleProviderFlow() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use this in SignInKickstarter
too? I like your method much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
public @StringRes int getGlobalTermsStringResource() { | ||
boolean hasTos = !TextUtils.isEmpty(termsOfServiceUrl); | ||
boolean hasPp = !TextUtils.isEmpty(privacyPolicyUrl); | ||
if(hasTos && hasPp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you run Ctrl + Alt + L
to auto-format this file? I died a little inside seeing if(
. 😁
Edit: Actually, I had a bunch of other style concerns so I've just created a patch to minimize the noise. Key takeaways:
- Whitespace matters ⬜️🌌
- There's nothing wrong with your annotation style, but our convention is to have them above the method
- If there's copypasta code, you're probably doing something wrong 😉. Take a look at how I got rid of your new
PreambleHandler
constructor and made thesetup
overload act as a utility for the actual implementation instead of duplicating it. - Consistency above all else: I'm usually a little nitpicky (sorry! 😊), but all I really care about is that new code is consistent with the rest of the codebase. Imagine trying to read a book that switches fonts every few sentences—the meaning of what you're reading would get lost in your brain going "Wait, whaaaat?" Or at least, that's what happens with me. 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been spoiled by auto formatting, sorry! Thank you for your comments, it is much appreciated.
} else if(hasTos) { | ||
return R.string.fui_tos_only; | ||
} else { | ||
return R.string.fui_pp_only; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else implies the default is the privacy policy—is that what you want? If so, we should add checks, update the README n' docs, throw exceptions, yada yada for AuthUI
to ensure the developer must supply either a tos or a pp. Otherwise, we should return -1 and handle that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I'll change this. We won't force them to have a ToS/Privacy policy though.
} | ||
|
||
public @StringRes int getGlobalTermsFooterStringResource() { | ||
return R.string.fui_tos_and_pp_footer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have if statements like above? If not, let's just inline it in the XML.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added 2 more strings to deal with ToS only & Pp only. Having this makes more sense now :)
@@ -130,4 +134,14 @@ private void setSlideAnimation() { | |||
// Make the next activity slide in | |||
overridePendingTransition(R.anim.fui_slide_in_right, R.anim.fui_slide_out_left); | |||
} | |||
|
|||
private void setUpTermsOfServiceFooter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing this method a lot. I wonder if it makes sense to add a PrivacyDisclosureUtils
class that includes this and the tos methods you added in FlowParameters
(I'm not a huge fan of logic in a POJO). @samtstern What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good idea to me, less repetition is better!
@@ -406,4 +409,14 @@ private SubmitConfirmationCodeFragment getSubmitConfirmationCodeFragment() { | |||
return (SubmitConfirmationCodeFragment) getSupportFragmentManager().findFragmentByTag | |||
(SubmitConfirmationCodeFragment.TAG); | |||
} | |||
|
|||
private void setUpTermsOfServiceFooter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm definitely thinking we need a utility, this is numeros quatros I've seen. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
@@ -32,6 +32,16 @@ | |||
android:id="@+id/btn_holder" | |||
style="@style/FirebaseUI.AuthMethodPicker.ButtonHolder" /> | |||
|
|||
<TextView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you meant to put this out of the scroll view, right? If not, you can't put more than one view in a ScrollView
so we'll need a LL.
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginTop="@dimen/fui_field_padding_vert" | ||
android:layout_gravity="bottom|right" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use end
instead of right
.
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginTop="@dimen/fui_field_padding_vert" | ||
android:textColor="?android:textColorTertiary" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the color change if you remove this? Either way, we need to use some AppCompat version, not the system one. I'll get back to you if the color is wrong when you remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment above addresses this I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to attr/textColorTertiary
@@ -100,4 +105,8 @@ | |||
<string name="fui_verify_phone_number" translation_description="Button text to submit confirmation code and complete phone verification">Verify Phone Number</string> | |||
<string name="fui_continue_phone_login" translation_description="Button text to submit phone number and send sms">Continue</string> | |||
<string name="fui_sms_terms_of_service" translation_description="Fine print warning displayed below Verify Phone Number button">By tapping “%1$s”, an SMS may be sent. Message & data rates may apply.</string> | |||
<string name="fui_sms_terms_of_service_and_privacy_policy_extended" translation_description="Fine print warning displayed below Verify Phone Number button">By tapping “%1$s”, you are indicating that you accept our <xliff:g example="https://google.com/terms" id="tos" translation_description="">%2$s</xliff:g> and <xliff:g example="https://google.com/privacy" id="pp" translation_description="">%3$s</xliff:g>. An SMS may be sent. Message & data rates may apply.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to need to kill fui_create_account_preamble_tos_and_pp
and the others to make lint happy. In the meantime, add 'MissingTranslation' // TODO remove in future PR
here and then @samtstern will probably have you create a second PR that kills those strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent off the strings for translation.
auth/src/main/res/values/strings.xml
Outdated
@@ -5,6 +5,11 @@ | |||
<string name="fui_default_toolbar_title" translatable="false">@string/app_name</string> | |||
<string name="fui_progress_dialog_loading" translation_description="Loading text in dialog">Loading…</string> | |||
<string name="fui_sign_in_default" translation_description="Button text to sign in">Sign in</string> | |||
<string name="fui_tos_and_pp" translation_description="Global ToS and Privacy policy message">By continuing, you are indicating that you accept our <xliff:g example="https://google.com/terms" id="tos" translation_description="">%1$s</xliff:g> and <xliff:g example="https://google.com/privacy" id="pp" translation_description="">%2$s</xliff:g>.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also make sure you add some translation_description that's not ""
.setPhotoUri(mUser.getPhotoUri()) | ||
.build()) | ||
.build(), | ||
new User.Builder(EmailAuthProvider.PROVIDER_ID, email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is something wrong with the formatting settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't worry about this. Equally legible either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, we just made an indentation change to the styling a long time ago and never bothered running it over the whole codebase. Like @samtstern said, we're not that nit picky. 😆 Thanks for checking though! 👍
I'll take another look at this tomorrow.
library/quality/quality.gradle
Outdated
@@ -77,6 +77,7 @@ android { | |||
'IconExpectedSize', | |||
'InvalidPackage', // Firestore uses GRPC which makes lint mad | |||
'NewerVersionAvailable', 'GradleDependency' // For reproducible builds | |||
'MissingTranslation' // TODO remove in future PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're also going to have to add UnusedResources
. And you need a comma above. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Almost all of my comments are about style or clarity and are very minor.
@@ -96,6 +99,19 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceStat | |||
} | |||
|
|||
view.findViewById(R.id.button_next).setOnClickListener(this); | |||
|
|||
TextView termsText = view.<TextView>findViewById(R.id.email_tos_and_pp_text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional style nit:
I normally see this:
TextView termsText = (TextView) view.findViewById(R.id.email_tos_and_pp_text);
Not this:
TextView termsText = view.<TextView>findViewById(R.id.email_tos_and_pp_text);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I forgot to mention something else about this. You don't need those casts! Notice how the IDE greys them out? That's cuz findViewById
was updated to use generics in API 26.
@@ -106,6 +108,11 @@ protected void onFailure(@NonNull Exception e) { | |||
} | |||
} | |||
}); | |||
|
|||
TextView termsText = findViewById(R.id.main_tos_and_pp); | |||
PrivacyDisclosureUtils.setupTermsOfServiceAndPrivacyPolicyText(AuthMethodPickerActivity.this, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do you need AuthMethodPickerActivity.this
or could it be just this
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change to this
Bundle savedInstanceState) { | ||
public View onCreateView(@NonNull LayoutInflater inflater, | ||
@Nullable ViewGroup container, | ||
@Nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: formatter kinda nuked this one. Can we put the Bundle savedInstanceState
back on the line with @Nullable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
mSmsTermsText, | ||
multipleProviderFlowText); | ||
|
||
if (!flowParameters.isSingleProviderFlow()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if this is a single provider flow then we'll have both the big text and the footer? If that's not the case (maybe the previous method has some internal switching) let's make that clearer here by doing if / else
so only one of the PrivacyDisclosureUtils
classes runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, SMS is special because we warn them either way (about the SMS cost). I'll change this
footerText); | ||
} | ||
|
||
public static void setupTermsOfServiceAndPrivacyPolicySmsText(Context context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok looking at this method I'd recommend a few things for code clarity:
- Drop the last parameter here
- Drop the
if/else
method in here - Where we call this method, do:
if (singleProvider) {
PrivacyUtils.blah()
} else {
termsText.setText(multipleMessage)
}
I think that's easier to read and more similar to how we use the non-SMS methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Changed it.
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginTop="@dimen/fui_field_padding_vert" | ||
android:textColor="?android:textColorTertiary" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment above addresses this I think.
android:textColor="?android:textColorTertiary" | ||
android:textIsSelectable="false" | ||
android:gravity="center" | ||
app:layout_constraintTop_toBottomOf="@id/container"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this with all numbers of buttons? IMO it would be more natural for this to be constrained Bottom_toBottomOf="parent"
so it's always exactly on the bottom.
Also make sure it looks OK in landscape.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I'll push these changes soon.
|
||
</ScrollView> | ||
|
||
<TextView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We repeat this a lot, we could extract most of the attributes into a style and then this would only be like a two-line copy paste (ID and style) into each layout instead of a ~10 line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll do this in a different PR.
android:layout_width="match_parent" | ||
android:layout_height="wrap_content" | ||
android:layout_marginTop="@dimen/fui_field_padding_vert" | ||
android:paddingRight="15dp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 15 is a strange number, try 16 which is a standard Android padding amount (normally you work in multiples of 4dp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -100,4 +105,8 @@ | |||
<string name="fui_verify_phone_number" translation_description="Button text to submit confirmation code and complete phone verification">Verify Phone Number</string> | |||
<string name="fui_continue_phone_login" translation_description="Button text to submit phone number and send sms">Continue</string> | |||
<string name="fui_sms_terms_of_service" translation_description="Fine print warning displayed below Verify Phone Number button">By tapping “%1$s”, an SMS may be sent. Message & data rates may apply.</string> | |||
<string name="fui_sms_terms_of_service_and_privacy_policy_extended" translation_description="Fine print warning displayed below Verify Phone Number button">By tapping “%1$s”, you are indicating that you accept our <xliff:g example="https://google.com/terms" id="tos" translation_description="">%2$s</xliff:g> and <xliff:g example="https://google.com/privacy" id="pp" translation_description="">%3$s</xliff:g>. An SMS may be sent. Message & data rates may apply.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent off the strings for translation.
@lsirac we've got some new lint errors |
Change-Id: Id996ce893941216beeb251be0b1fa2b076c220a2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more nits. Besides those, LGTM!
PS: lint thinks you have some pointless FrameLayout
s, dunno if that's true.
@@ -92,9 +92,8 @@ public void onComplete(@NonNull Task<CredentialRequestResponse> task) { | |||
|
|||
private void startAuthMethodChoice() { | |||
List<AuthUI.IdpConfig> idpConfigs = getArguments().providerInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we inline getArguments().providerInfo
below? (Trick: use Ctrl + Shift + N
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. Ctrl + Shift + N lets me search for files.
I changed it so that there is no local idpConfigs list, we just do getArguments().providerInfo.get(0) instead. Is that what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, my B. I meant Ctrl + Alt + N
. 😊 You should try it, I'm always impressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yeah, LGTM
|
||
TextView termsText = view.findViewById(R.id.email_tos_and_pp_text); | ||
TextView footerText = view.findViewById(R.id.email_footer_tos_and_pp_text); | ||
FlowParameters flowParameters = getFlowParams(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of flowParameters
? (It's just as long as getFlowParams()
and is only used once.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to pass flowParameters instead of making 2 more calls to getFlowParams() - unless you want me to change them all to getFlowParams()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, SGTM
|
||
private static final int NO_TOS_OR_PP = -1; | ||
|
||
public static void setupTermsOfServiceAndPrivacyPolicyText(Context context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should shorten this to setupTosAndPp
. Or at least, setupTermsOfServiceAndPrivacyPolicyText
is quite a mouthful. 😆 @samtstern Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep it like this 😊, do you both want me to change it? @samtstern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think that's clearer then SGTM 👍.
} else if (privacyPolicyUrlProvided) { | ||
return R.string.fui_pp_footer; | ||
} | ||
return NO_TOS_OR_PP; // add constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// add constant
Looks like you did that, comment no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. 😶
private static int getGlobalTermsStringResource(FlowParameters flowParameters) { | ||
boolean termsOfServiceUrlProvided = flowParameters.isTermsOfServiceUrlProvided(); | ||
boolean privacyPolicyUrlProvided = flowParameters.isPrivacyPolicyUrlProvided(); | ||
if (termsOfServiceUrlProvided && privacyPolicyUrlProvided) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a new line before and after the if/else here and in the other methods. Makes it more readable IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
android:textIsSelectable="false" | ||
tools:text="Terms and Privacy Policy" /> | ||
|
||
<TextView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between this TextView and the one above? If this isn't a mistake, we should add a comment explaining that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Oh hey, looks like @samtstern fixed them! |
@SUPERCILEX the layouts are pointless but they're gonna need to be there for my progress dialog thing anyway so just making my merge life easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from me, just a few more comments from @SUPERCILEX
Alrighty, waiting for you to push! |
Merged, welcome to the contributors list @lsirac! |
Changes toward GDPR compliance (#1245)
Still need to add a ToS/Pp footer nearly everywhere.