808152 - [App Icons] Need resized app icons from 64 to 60px.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect, P3)

Comment 1

[Patryk Adamczyk [:patryk] UX]

60 x 60 px is the new spec based on issues found when adding 3rd party apps.

Comment 2

[Peter La]

Attachment: https://github.com/mozilla-b2g/gaia/pull/6339

https://github.com/mozilla-b2g/gaia/pull/6339

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 680265 [details]
https://github.com/mozilla-b2g/gaia/pull/6339

Please also change this line right here

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/appmanager.js#L275

properly test your code, and ask for review again.

Comment 4

[Ben Francis [:benfrancis]]

This bug is marked as depending on bug 808152 which has an attached patch which differs from this one by one line of JavaScript. I'm guessing you branched that one off this one.

r+me with the change requested by Tim and if these two are landed together (by merging pull request 6340 from the other bug).

Comment 5

[Peter La]

Attachment: Pull Request 6364

@timdream @benfrancis

I deleted my original branches, please ignore the old requests.
This should be clean now - 1 branch for image updates, 1 branch for .js file updates.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 680728 [details]
Pull Request 6364

Thanks!

Comment 7

[Peter La]

Hi :timdream :benfrancis,

I checked github and it looks like this hasn't been merged yet.  Is it just a matter of time or is there something wrong about my change?

Thanks.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Peter La from comment #7)
> Hi :timdream :benfrancis,
> 
> I checked github and it looks like this hasn't been merged yet.  Is it just
> a matter of time or is there something wrong about my change?
> 
> Thanks.

Peter, the attached PR has been closed 3 days ago. Was is intentional or is there a new PR somewhere?

Comment 9

[Ben Francis [:benfrancis]]

Looks like you split the pull request into:
https://github.com/mozilla-b2g/gaia/pull/6364
https://github.com/mozilla-b2g/gaia/pull/6365

The latter is waiting on changes following the review by crdlc and has bitrotted so can't be automatically merged.

Ideally these two patches need landing together.

Comment 10

[Peter La]

@vingtetun @benfrancis

Yes I realized later that I should have just made one request, one check-in.  It would have been simpler because they are so interdependant.

Is there anything I need to do to make sure these get landed or do I simply wait?

Comment 11

[Ben Francis [:benfrancis]]

Please see comment 9, you need to address the review comments from crdlc and your patch is out of date so you need to merge upstream changes because the pull request can't currently be merged due to potential conflicts.

Comment 12

[Peter La]

Thanks Ben.

Ok, I've read the comments.  Would it be ok if I resync to master, make a new branch, and make all changes on one commit (images + js), and submit a new pull request?

I'm no expert on git/github... so it seems like that would be the easiest way to test/fix things if I were to do it.  If on the other hand, one of you guys would like to help me out, that would be great also. :)

Comment 13

[Peter La]

I've redone this change by syncing to latest version of master, creating one new branch (appicon_updateandresize) and making all changes to it (including the ones crdlc asked for + additional ones I found necessary), and submitted a new pull request to replace the previous 2.

Please review and approve pull request https://github.com/mozilla-b2g/gaia/pull/6537.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/b9d1d6f9b01661db17a9d932d614fd9a1d79a720