Skip to content
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

[Enhancement] - Change Settings > Help web views to Chrome Custom Tabs #437

Merged
merged 14 commits into from
Feb 1, 2019

Conversation

Rcureton
Copy link
Contributor

@Rcureton Rcureton commented Jan 18, 2019

What ❓

  • Changed all of the Help links that were previously using WebViews and refactored them to using ChromeTabs which give these links a more native feel and faster loading.

  • Unfortunately Chrome Tabs doesn't support injecting headers as per this article Chrome tabs and headers. Also I referenced the documentation on Chrome tabs and there's nothing for injecting headers as well.

  • I used this article as a reference for implementation Chrome Custom Tabs

Story 📖

Change Settings > Help web views to Chrome Custom Tabs

See 👀

@Rcureton Rcureton changed the title [Enhancement [Enhancement] - Change Settings > Help web views to Chrome Custom Tabs Jan 18, 2019
@Rcureton Rcureton requested a review from eoji January 18, 2019 19:08
@Rcureton
Copy link
Contributor Author

Copy link
Contributor

@eoji eoji left a comment

Choose a reason for hiding this comment

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

The toolbar and status bar are green in my build. Can you please update the screenshots.

These screens are not only used in settings. They're also in the sign up/log in flow and checkout. So right now this feels janky, half open in a custom chrome tab and the other in our custom webview @nnekab @dannyalright.

It also crashes if a user doesn't have a browser that supports custom tabs.

@@ -85,4 +74,18 @@ class HelpSettingsActivity : BaseActivity<HelpSettingsViewModel.ViewModel>() {
startActivity(Intent.createChooser(intent, getString(R.string.support_email_chooser)))
}
}

private fun loadChromePages(endpoint: String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename this to either startChromeTab or startCustomTab for accuracy.

builder.setToolbarColor(ContextCompat.getColor(this, R.color.primary))

val customTabsIntent = builder.build()
customTabsIntent.launchUrl(this, Uri.parse(url))
Copy link
Contributor

Choose a reason for hiding this comment

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

This crashes if the user doesn't have a browser that's capable of handling a custom tabs. I don't think we needed to remove all of that code. We need a fallback.

terms_of_use.setOnClickListener {
startActivityWithSlideLeftTransition(Intent(this, HelpActivity.Terms::class.java))
}
cookie_policy.setOnClickListener{ loadChromePages("cookies") }
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be constants. We previously used Secrets.HelpCenter.ENDPOINT and the static classes in HelpActivity

Copy link
Contributor

@eoji eoji left a comment

Choose a reason for hiding this comment

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

Code looks good but can we please update the screenshot. Here's what it looks like on my Pixel 3 (Pie):

@Rcureton
Copy link
Contributor Author

Rcureton commented Feb 1, 2019

Done!

@Rcureton Rcureton merged commit 397e999 into master Feb 1, 2019
@Rcureton Rcureton deleted the rc/chrome-tabs branch February 1, 2019 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants