Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #2205: View a tab group #2249

Merged
merged 9 commits into from
May 6, 2019
Merged

Conversation

sblatz
Copy link
Contributor

@sblatz sblatz commented May 2, 2019

This PR is almost complete, but I want to do a round of clean up on it before review!

@sblatz sblatz requested a review from a team as a code owner May 2, 2019 23:03
@sblatz sblatz removed the request for review from a team May 2, 2019 23:05
Copy link
Contributor

@ekager ekager left a comment

Choose a reason for hiding this comment

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

Overall, looks great!

app/src/main/java/org/mozilla/fenix/HomeActivity.kt Outdated Show resolved Hide resolved
@@ -0,0 +1,6 @@
<vector android:height="20dp" android:viewportHeight="20"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit- license


<shape xmlns:android="http://schemas.android.com/apk/res/android"
android:shape="rectangle">
<solid android:color="@color/photonGrey30" />
Copy link
Contributor

Choose a reason for hiding this comment

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

at some point we should probably update all of these to have an attr color but doesn't have to be now

@@ -21,7 +21,16 @@ fun String.replace(pairs: Map<String, String>): String {
fun String?.urlToHost(): String {
return try {
val url = URL(this)
url.host
val firstIndex = url.host.indexOfFirst { it == '.' } + 1
val lastIndex = url.host.indexOfLast { it == '.' }
Copy link
Contributor Author

@sblatz sblatz May 6, 2019

Choose a reason for hiding this comment

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

@ekager this is the updated way I'm generating the "host" string because just want the name of the website (e.g. mozilla instead of www.mozilla.org like we previously had.)

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 cleaned this up quite a bit so it looks nicer and has a comment to explain the logic. Do you think it's okay to have this change here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only place this extension is used? You may want to create another function just to not break someone else's code using 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.

Good point. I moved it to a separate function :)

@sblatz sblatz changed the title [WIP] Create tab group Create tab group May 6, 2019
@sblatz sblatz changed the title Create tab group For #2205: View a tab group May 6, 2019
@sblatz sblatz force-pushed the create-tab-group branch 2 times, most recently from 221824d to f3494a2 Compare May 6, 2019 17:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants