Closed Bug 1366669 Opened 5 years ago Closed 5 years ago

(photon) update icons

Categories

(Firefox for Android Graveyard :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed, firefox57 verified)

VERIFIED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: wesley_huang, Assigned: jwu)

References

Details

Attachments

(3 files)

No description provided.
Comment on attachment 8886076 [details]
Bug 1366669 - Part 3: Check correct resource folder according to current skin.

https://reviewboard.mozilla.org/r/156876/#review162184

I have some nits, and one minor request, but this doesn't need re-review.

::: commit-message-e2e47:1
(Diff revision 1)
> +Bug 1366669 - Part 3: Check correct resource folder accroding to current skin. r?nalexander,walkingice

nit: s/accroding/according/

::: commit-message-e2e47:3
(Diff revision 1)
> +Bug 1366669 - Part 3: Check correct resource folder accroding to current skin. r?nalexander,walkingice
> +
> +Check images in photon/res or australis/res based on `--enable-photon` is set or not in mozconfig.

nit: based on whether `--enable-photon` is set ... (missing whether)

::: mobile/android/base/locales/Makefile.in:98
(Diff revision 1)
>  # L10NBASEDIR is not defined for en-US.
>  l10n-srcdir := $(if $(filter en-US,$(AB_CD)),,$(or $(realpath $(L10NBASEDIR)),$(abspath $(L10NBASEDIR)))/$(AB_CD)/mobile/chrome)
>  
>  $(eval $(call generated_file_template,suggestedsites,suggestedsites.json))
>  
> +ifdef MOZ_ANDROID_PHOTON

We're not consistent (yet), but prefer to use lower-case names for local variables and upper-case names for global variables.  Also, in this case you can use `$(if)`, I think -- can you try:

```
skin := $(if $(MOZ_ANDROID_PHOTON),photon,australis)
```
Attachment #8886076 - Flags: review?(nalexander) → review+
Comment on attachment 8886074 [details]
Bug 1366669 - Part 1: Rename images and move to Australis folder.

https://reviewboard.mozilla.org/r/156872/#review162386

::: mobile/android/app/src/main/res/drawable/close_edit_mode_selector.xml:10
(Diff revision 2)
>  
>  <selector xmlns:android="http://schemas.android.com/apk/res/android"
>            xmlns:gecko="http://schemas.android.com/apk/res-auto">
>  
>      <item gecko:state_dark="true"
> -          android:drawable="@drawable/close_edit_mode_dark"/>
> +          android:drawable="@drawable/ic_cancel_nm"/>

I guess we can just remove list line since lightweight theme is not awared.

::: mobile/android/app/src/main/res/drawable/close_edit_mode_selector.xml:15
(Diff revision 2)
> -          android:drawable="@drawable/close_edit_mode_dark"/>
> +          android:drawable="@drawable/ic_cancel_nm"/>
>  
>      <item gecko:state_private="true"
> -          android:drawable="@drawable/close_edit_mode_light"/>
> +          android:drawable="@drawable/ic_cancel_pm"/>
>  
> -    <item android:drawable="@drawable/close_edit_mode_light"/>
> +    <item android:drawable="@drawable/ic_cancel_pm"/>

by default using *pm*?
Attachment #8886074 - Flags: review?(walkingice0204) → review+
Comment on attachment 8886075 [details]
Bug 1366669 - Part 2: Update icons for Photon.

https://reviewboard.mozilla.org/r/156874/#review162392
Attachment #8886075 - Flags: review?(walkingice0204) → review+
Comment on attachment 8886076 [details]
Bug 1366669 - Part 3: Check correct resource folder according to current skin.

https://reviewboard.mozilla.org/r/156876/#review162394
Attachment #8886076 - Flags: review?(walkingice0204) → review+
Assignee: nobody → topwu.tw
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/41cdb9589102
Part 1: Rename images and move to Australis folder. r=walkingice
https://hg.mozilla.org/integration/autoland/rev/d97403d6b52d
Part 2: Update icons for Photon. r=walkingice
https://hg.mozilla.org/integration/autoland/rev/2b56008c121a
Part 3: Check correct resource folder according to current skin. r=nalexander,walkingice
Keywords: checkin-needed
Depends on: 1381501
Blocks: 1387349
Verified as fixed on Nightly 57. Icons have been updated following the design.
Status: RESOLVED → 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.