Closed Bug 872762 Opened 7 years ago Closed 6 years ago

Add a PagerTabStrip for the HomePager

Categories

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

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: sriram, Assigned: sriram)

References

Details

(Whiteboard: fixed-fig)

Attachments

(1 file, 1 obsolete file)

We need a pager-tab-strip for the HomePager to indicate the page we are in.
Attached patch Patch (obsolete) — Splinter Review
This adds a PagerTabStrip from the support library.
Attachment #750083 - Flags: review?(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?
Attachment #750083 - Flags: review?(margaret.leibovic) → review-
(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.
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.
Attached patch PatchSplinter Review
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.
Attachment #750083 - Attachment is obsolete: true
Attachment #750166 - Flags: review?(margaret.leibovic)
Comment on attachment 750166 [details] [diff] [review]
Patch

Nice, I like this more.
Attachment #750166 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/0f895e8c3d7f
Assignee: nobody → sriram
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.