Closed Bug 1366680 Opened 7 years ago Closed 7 years ago

(photon) Tab tray UI refresh and consistency

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox57 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: wesley_huang, Assigned: cnevinchen)

References

Details

(Whiteboard: [FNC][SPT57.1][MVP])

User Story

Visual spec - https://drive.google.com/file/d/0B-9PIePlQZRldk9INm9TUXBkcWs/view?usp=sharing
Assets - https://drive.google.com/open?id=0B-9PIePlQZRlbGJ6RWp6bkl5RU0

Attachments

(1 file)

Bug 1366680 - Refresh Tab Tray UI.
59 bytes, text/x-review-board-request
jwu
: review+
walkingice
: review+
Details 
      No description provided.
Assignee: nobody → cnevinchen
User Story: (updated)
User Story: (updated)
Hi jwu
I saw bug 1372486 is landed. I guess I don't need https://github.com/idiotmax/gecko/tree/photon_zoo?
I got some build error when building the github feature branch.

0:26.28 Error running mach:
0:26.28
0:26.28     ['--log-no-times', 'artifact', 'install']
0:26.28
0:26.28 The error occurred in code that was called by the mach command. This is either
0:26.28 a bug in the called code itself or in the way that mach is calling it.
0:26.28
0:26.29 You should consider filing a bug for this issue.
0:26.29
0:26.29 If filing a bug, please include the full output of mach, including this error
0:26.29 message.
0:26.30
0:26.30 The details of the failure are as follows:
0:26.30
0:26.30 IndexError: list index out of range
0:26.30
0:26.30   File "/Users/nechen/Desktop/mozilla-central/python/mozbuild/mozbuild/mach_commands.py", line 1598, in artifact_install
0:26.30     return artifacts.install_from(source, self.distdir)
0:26.30   File "/Users/nechen/Desktop/mozilla-central/python/mozbuild/mozbuild/artifacts.py", line 1149, in install_from
0:26.30     return self.install_from_recent(distdir)
0:26.30   File "/Users/nechen/Desktop/mozilla-central/python/mozbuild/mozbuild/artifacts.py", line 1110, in install_from_recent
0:26.30     return self._install_from_hg_pushheads(hg_pushheads, distdir)
0:26.30   File "/Users/nechen/Desktop/mozilla-central/python/mozbuild/mozbuild/artifacts.py", line 1089, in _install_from_hg_pushheads
0:26.30     for trees, hg_hash in hg_pushheads:
0:26.30   File "/Users/nechen/Desktop/mozilla-central/python/mozbuild/mozbuild/artifacts.py", line 990, in _find_pushheads
0:26.30     candidate_pushheads = self._pushheads_from_rev(last_revs[0].rstrip(),
0:26.31 make[4]: *** [recurse_artifact] Error 1
0:26.31 make[3]: *** [artifact] Error 2
0:26.31 make[2]: *** [default] Error 2
0:26.31 make[1]: *** [realbuild] Error 2
0:26.31 make: *** [build] Error 2
0:26.32 0 compiler warnings present.


Can you also let me know where is the photon colors you previous mentioned?
e.g. #272727 or #F9F9FA in the attachment.
Or should I create my own in colors.xml ?
Flags: needinfo?(topwu.tw)
It looks like an artifact build error, maybe clear files in `$HOME/.mozbuild/package-frontend` and try again?

Also the colors for Photon aren't landed yet, you can use commits in https://github.com/idiotmax/gecko/tree/photon_zoo for temporary.
Flags: needinfo?(topwu.tw)
Blocks: 1379655
Hi Carol
If the update design is done, please let me know :) Thanks!
Flags: needinfo?(chuang)
Hi Nevin,
I just updated the spec to the Gdrive! 
v 0.4 Revised TAB TRAY UI -p.11 , p.12

please take a look and let me know if you have any other question :)
Flags: needinfo?(chuang) → needinfo?(cnevinchen)
Thanks~!
Flags: needinfo?(cnevinchen)
Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

https://reviewboard.mozilla.org/r/156902/#review166634

::: mobile/android/app/src/australis/res/values/colors.xml:111
(Diff revision 8)
>  
> +  <!-- Tab Tray -->
> +  <color name="tab_item_normal_bg">#F9F9FA</color>
> +  <color name="tab_item_private_bg">#38383D</color>
> +  <color name="tab_item_normal_highlight_bg">#0a84ff</color>
> +  <color name="tab_item_normal_highlight_bg_80">#CC0a84ff</color>

I don't understand what does '80' mean

::: mobile/android/app/src/australis/res/values/colors.xml:113
(Diff revision 8)
> +  <color name="tab_item_normal_bg">#F9F9FA</color>
> +  <color name="tab_item_private_bg">#38383D</color>
> +  <color name="tab_item_normal_highlight_bg">#0a84ff</color>
> +  <color name="tab_item_normal_highlight_bg_80">#CC0a84ff</color>
> +  <color name="tab_item_private_highlight_bg">#AC39FF</color>
> +  <color name="tab_item_private_highlight_bg_80">#CCAC39FF</color>

same here

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java:54
(Diff revision 8)
>      }
>  
> +    private void setPadding() {
> +        final float scale = getResources().getDisplayMetrics().density;
> +        final float sizeInDp = getResources().getDimension(R.dimen.tab_panel_list_item_top_padding);
> +        int dpAsPixels = (int) (sizeInDp * scale + 0.5f);

nit: final

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java:55
(Diff revision 8)
>  
> +    private void setPadding() {
> +        final float scale = getResources().getDisplayMetrics().density;
> +        final float sizeInDp = getResources().getDimension(R.dimen.tab_panel_list_item_top_padding);
> +        int dpAsPixels = (int) (sizeInDp * scale + 0.5f);
> +        setPadding(0, dpAsPixels, 0, dpAsPixels);

what if we explicit say `super.setPadding` ?

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsPanel.java:121
(Diff revision 8)
>      private IconTabWidget mTabWidget;
>      private View mMenuButton;
>      private ImageButton mAddTab;
>  
> +    // Tab Tray
> +    @Nullable private ThemedImageButton normalTabsPanel;

this naming is inconsistent with other members in this class

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsPanel.java:122
(Diff revision 8)
>      private View mMenuButton;
>      private ImageButton mAddTab;
>  
> +    // Tab Tray
> +    @Nullable private ThemedImageButton normalTabsPanel;
> +    @Nullable private ThemedImageButton privateTabsPanel;

same here

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsPanel.java:172
(Diff revision 8)
>          });
>  
>          mTabWidget = (IconTabWidget) findViewById(R.id.tab_widget);
>  
> -        mTabWidget.addTab(R.drawable.tabs_normal, R.string.tabs_normal);
> -        final ThemedImageButton privateTabsPanel =
> +        final View tabNormal = mTabWidget.addTab(R.drawable.tabs_normal, R.string.tabs_normal);
> +        normalTabsPanel = tabNormal instanceof ThemedImageButton ? ((ThemedImageButton) tabNormal) : null;

the original implementation already assumes it should be an ThemedImageButton. If it is not, we can easily catch it in development stage. So I am not sure Type and Null checking are necessary or not. But it won't hurt anyone so I am still good with current implementation.
Attachment #8886104 - Flags: review?(walkingice0204) → review+
Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

https://reviewboard.mozilla.org/r/156902/#review167212

::: mobile/android/app/src/australis/res/values/colors.xml:114
(Diff revision 9)
> +  <color name="tab_item_private_bg">#38383D</color>
> +  <color name="tab_item_normal_highlight_bg">#0a84ff</color>
> +  <color name="tab_item_normal_highlight_bg_80">#CC0a84ff</color>
> +  <color name="tab_item_private_highlight_bg">#AC39FF</color>
> +  <color name="tab_item_private_highlight_bg_80">#CCAC39FF</color>
> +  <color name="tab_item_thumbnail_default_bg">#E4E4E4E4</color>

Should this be #E4E4E4(three "E4"s) instead of #E4E4E4E4(four "E4"s)?

::: mobile/android/app/src/photon/res/values/colors.xml:203
(Diff revision 9)
> +    <color name="tab_item_private_bg">#38383D</color>
> +    <color name="tab_item_normal_highlight_bg">#0a84ff</color>
> +    <color name="tab_item_normal_highlight_bg_80">#CC0a84ff</color>
> +    <color name="tab_item_private_highlight_bg">#AC39FF</color>
> +    <color name="tab_item_private_highlight_bg_80">#CCAC39FF</color>
> +    <color name="tab_item_thumbnail_default_bg">#E4E4E4E4</color>

Same question as above.

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java:52
(Diff revision 9)
> +        final float scale = getResources().getDisplayMetrics().density;
> +        final float sizeInDp = getResources().getDimension(R.dimen.tab_panel_list_item_top_padding);
> +        final int dpAsPixels = (int) (sizeInDp * scale + 0.5f);

Why not use `getDimensionPixelSize()`?, The result is rounded, which means +0.5f might not be needed.

https://developer.android.com/reference/android/content/res/Resources.html#getDimensionPixelSize(int)
Attachment #8886104 - Flags: review?(topwu.tw) → review+
Comment on attachment 8886104 [details]
Bug 1366680 - Refresh Tab Tray UI.

https://reviewboard.mozilla.org/r/156902/#review166634

> I don't understand what does '80' mean

it means 80% transparency.

> same here

it means 80% transparency.
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4861ce4f82f6
Refresh Tab Tray UI. r=jwu,walkingice
https://hg.mozilla.org/mozilla-central/rev/4861ce4f82f6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Whiteboard: [FNC][SPT#57.1][MVP]
Whiteboard: [FNC][SPT#57.1][MVP] → [FNC][SPT57.1][MVP]
Verified as fixed on Nightly 56.0a1, (2017-08-11).
Devices: 
Huawei Honor 8 (Android 6.0)
Motorola Nexus 6 (Android 7.0)
Prestigio Grace X5 (Android 4.4.2)
Huawei Honor 8 (Android 6.0)
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: