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)
Firefox for Android Graveyard
General
Tracking
(firefox57 verified)
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: wesley_huang, Assigned: cnevinchen)
References
Details
(Whiteboard: [FNC][SPT57.1][MVP])
User Story
Attachments
(1 file)
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → cnevinchen
Updated•7 years ago
|
User Story: (updated)
Updated•7 years ago
|
User Story: (updated)
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
Hi Carol If the update design is done, please let me know :) Thanks!
Flags: needinfo?(chuang)
Comment 5•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review-reply |
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.
Comment 20•7 years ago
|
||
Pushed by nechen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4861ce4f82f6 Refresh Tab Tray UI. r=jwu,walkingice
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4861ce4f82f6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•7 years ago
|
Whiteboard: [FNC][SPT#57.1][MVP]
Updated•7 years ago
|
Whiteboard: [FNC][SPT#57.1][MVP] → [FNC][SPT57.1][MVP]
Comment 22•7 years ago
|
||
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
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•