872762 - Add a PagerTabStrip for the HomePager

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

We need a pager-tab-strip for the HomePager to indicate the page we are in.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This adds a PagerTabStrip from the support library.

Comment 2

[:Margaret Leibovic]

Comment on attachment 750083 [details] [diff] [review]
Patch

Review of attachment 750083 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! This looks good, but I'm just giving an r- for now to get answers to the questions below before we land this.

::: mobile/android/base/BrowserApp.java
@@ +364,5 @@
>              }
>          });
>  
>          mHomePager = (HomePager) findViewById(R.id.abouthome_pager);
> +        ((PagerTabStrip) findViewById(R.id.abouthome_pager_title)).setTabIndicatorColor(0xFFFF9500);

Can this be done in HomePager instead of in BrowserApp? I think it would be nicer to keep that in there.

Also, can we factor out this color to colors.xml?

::: mobile/android/base/home/HomePager.java
@@ +134,5 @@
>  
>          @Override
> +        public CharSequence getPageTitle(int position) {
> +            TabInfo info = mTabs.get(position);
> +            return info.title.toUpperCase();

Is there a reason for upper casing the title here?

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +33,5 @@
>                                                android:layout_height="fill_parent"
>                                                android:background="@color/background_normal"
> +                                              android:visibility="gone">
> +
> +                <android.support.v4.view.PagerTabStrip android:id="@+id/abouthome_pager_title"

This view handles the whole tab strip, not just an individual title, so I think a better id would be something like "home_pager_tab_strip".

I also think the HomePager id should be renamed home_pager, since we've been moving away from calling things AboutHomeWhatever, but that's not in the scope of this bug :)

::: mobile/android/base/resources/values/styles.xml
@@ +157,5 @@
>          <item name="android:textColor">@color/primary_text</item>
>      </style>
>  
> +    <style name="TextAppearance.Widget.PagerTabStrip" parent="TextAppearance.Small">
> +        <item name="android:textColor">#FF666666</item>

Same thing here - should we put this in colors.xml?

Comment 3

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #2)
> Comment on attachment 750083 [details] [diff] [review]
> Patch
> 
> Review of attachment 750083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! This looks good, but I'm just giving an r- for now to get answers to
> the questions below before we land this.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +364,5 @@
> >              }
> >          });
> >  
> >          mHomePager = (HomePager) findViewById(R.id.abouthome_pager);
> > +        ((PagerTabStrip) findViewById(R.id.abouthome_pager_title)).setTabIndicatorColor(0xFFFF9500);
> 
> Can this be done in HomePager instead of in BrowserApp? I think it would be
> nicer to keep that in there.
> 
> Also, can we factor out this color to colors.xml?

We can't do this in HomePager. I had a different-better idea for this. But then felt too bored to do it. :P

Basically have a "HomePagerTabStrip" extend "PagerTabStrip" that exposes the attribute "tabIndicatorColor", which can be set in XML, and it takes care of calling super with it. That way, we don't want to have id's and stuff. Is that approach fine with you?

> 
> ::: mobile/android/base/home/HomePager.java
> @@ +134,5 @@
> >  
> >          @Override
> > +        public CharSequence getPageTitle(int position) {
> > +            TabInfo info = mTabs.get(position);
> > +            return info.title.toUpperCase();
> 
> Is there a reason for upper casing the title here?
> 

Yes. Though PagerTabStrip can take a "textAllCaps" argument, its available only 14+. This would crash in older phones. Hence it's better to make it upper case in code than in XML.

> ::: mobile/android/base/resources/layout/gecko_app.xml
> @@ +33,5 @@
> >                                                android:layout_height="fill_parent"
> >                                                android:background="@color/background_normal"
> > +                                              android:visibility="gone">
> > +
> > +                <android.support.v4.view.PagerTabStrip android:id="@+id/abouthome_pager_title"
> 
> This view handles the whole tab strip, not just an individual title, so I
> think a better id would be something like "home_pager_tab_strip".
> 

If you are fine with my "HomePageTitleStrip" approach, we don't need an id here.

> I also think the HomePager id should be renamed home_pager, since we've been
> moving away from calling things AboutHomeWhatever, but that's not in the
> scope of this bug :)
> 
> ::: mobile/android/base/resources/values/styles.xml
> @@ +157,5 @@
> >          <item name="android:textColor">@color/primary_text</item>
> >      </style>
> >  
> > +    <style name="TextAppearance.Widget.PagerTabStrip" parent="TextAppearance.Small">
> > +        <item name="android:textColor">#FF666666</item>
> 
> Same thing here - should we put this in colors.xml?

I don't want to push this to colors.xml as we don't use this color anywhere else. Or may be I can do a "?android:attr/textColorHint", if that's fine with you.

Comment 4

[:Margaret Leibovic]

Thanks for answering my questions, I mostly wanted to make sure we were thinking about these issues. The HomePagerTabStrip approach sounds good to me, although I'll let you decide if you think it's worth it or not. We can always file follow-ups to change things if we change our minds later.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Updated for the comments. Leaving one color in gecko_app.xml. It's not used anywhere else. If we re-use it (I don't see in his mockups), we can refactor it and move it to colors.xml. Otherwise this looks optimal to me.

Comment 6

[:Margaret Leibovic]

Comment on attachment 750166 [details] [diff] [review]
Patch

Nice, I like this more.

Comment 7

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/0f895e8c3d7f

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0f895e8c3d7f