OS Doesn't properly use privacy_sprite.png in Settings

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: pla, Assigned: sjochimek)

Tracking

unspecified
All
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g18+ affected)

Details

(Whiteboard: visual design incorrect implementation UX-P2 Berlinww, uxbranch, landed in uxbranch, qa-verified)

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
We're replacing the branded and unbranded assets in:

gaia/shared/resources/branding

and there are two pngs that are not being used by Settings to display its icons in Setting -> Device Information -> Privacy:
official/privacy_sprite.png
unofficial/privacy_sprite.png

The assets will be checked in with Bug 828251, so I've noted the dependency.
(Assignee)

Updated

6 years ago
Assignee: nobody → sjochimek
(Reporter)

Updated

6 years ago
No longer depends on: 828251
(Reporter)

Updated

6 years ago
Depends on: 828251
(Assignee)

Comment 1

6 years ago
Created attachment 702741 [details]
patch

This patch use the privacy_sprite.png in shared folder.
Attachment #702741 - Flags: feedback?(pla)
(Reporter)

Comment 2

6 years ago
Hey Sam,

I looked at your fix, and I believe this is not the correct way to fix it.  You can't just reference one image, you have to reference the correct version, either branded (official) or unbranded (unofficial).  There are two directories under gaia/shared/resources/branding, each with its own copy of privacy_sprite.png:

gaia/shared/resources/branding/official -> image contains the firefox logo
gaia/shared/resources/branding/unofficial -> image contains a generic blue globe

I checked in new images for these yestereday with:
https://github.com/gordonbrander/gaia/pull/92

Can you take a look at this again and make the proper changes?

Thanks!
Peter
(Assignee)

Comment 3

6 years ago
Created attachment 702844 [details]
render official/unofficial icons in privacy list

Peter, 

Actually there is a flag MOZILLA_OFFICIAL=1 that you can use to build gaia using official resources or not.

That's why in HTML/CSS we use paths without the official/unofficial part.

Here is the command you should try to build gaia on the device: MOZILLA_OFFICIAL=1 make production

Can you test with that and let me know if i doing something wrong ?
Flags: needinfo?(pla)
(Reporter)

Comment 4

6 years ago
Hi Sam,

Thanks for clarifying.  As long as the graphics in those official/unofficial directories are still being used based on the flag.  I was just worried that you moved the graphics to the directory above. :)

Peter.
Flags: needinfo?(pla)
(Assignee)

Updated

6 years ago
Attachment #702741 - Flags: feedback?(pla)
(Assignee)

Comment 5

6 years ago
Landed in uxbranch: https://github.com/gordonbrander/gaia/commit/081e4e75e15d50754cc0dda652bd6ddacc397d6a
Whiteboard: visual design incorrect implementation UX-P2 Berlinww → visual design incorrect implementation UX-P2 Berlinww, uxbranch, landed in uxbranch
Sweet.  qa-verified
Whiteboard: visual design incorrect implementation UX-P2 Berlinww, uxbranch, landed in uxbranch → visual design incorrect implementation UX-P2 Berlinww, uxbranch, landed in uxbranch, qa-verified
The specific master commit for this app is 9aff5a6 .

Should this go to v1 ?
tracking-b2g18: --- → ?
If the UX team is making a case for this in v1.1 go ahead with nomination for uplift.
tracking-b2g18: ? → +
Not fixed on master : 
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/icons.css#L351

nor v1 train :
https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/settings/style/icons.css#L318

I think someone overwrote the changes to the master one and it probably never got pushed to v1 train.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-b2g18: --- → affected
(In reply to Naoki Hirata :nhirata from comment #10)
> Not fixed on master : 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/icons.
> css#L351

actually this line was not in the original patch, it was introduced later.

> 
> nor v1 train :
> https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/settings/style/icons.
> css#L318

It was never uplifted to v1-train because nobody cared enough.

> 
> I think someone overwrote the changes to the master one and it probably
> never got pushed to v1 train.

Yep, it seems that was Bug 830644.
see https://github.com/mozilla-b2g/gaia/commit/0f3a6c729085e322502c05b5dc932bf27ea0139c#L4L323

But in that patch, the good image was actually put in settings and I actually just verified that it works as expected in master. Could you check again ?
I can't check from a visual stand point even though the code looks right.  see bug 858722
(Assignee)

Comment 13

6 years ago
Created attachment 738987 [details]
Regression fix RP
Attachment #738987 - Flags: review?(felash)
Comment on attachment 738987 [details]
Regression fix RP

added a comment in the PR

sorry about the delay, I was busy with tef+ stuff.
still waiting for the follow up
(Assignee)

Comment 16

6 years ago
Just updated the branch.
Comment on attachment 738987 [details]
Regression fix RP

r=me
thanks Sam !
Attachment #738987 - Flags: review?(felash) → review+
Sam, could you also follow that it works as expected on the v1-train branch ? Like requesting approval on both patches, and resolving conflicts ?
Flags: needinfo?(sjochimek)
(Assignee)

Comment 19

6 years ago
Follow up merge in master: https://github.com/mozilla-b2g/gaia/commit/dbe4a990391a8045dc1caf0241ad3926126b0ad8
Flags: needinfo?(sjochimek)
(Assignee)

Comment 20

6 years ago
Actually the first commit https://github.com/mozilla-b2g/gaia/commit/9aff5a63a7f3e4609e939f90847640274a77501a hasn't any conflicts with v1-train. 

Should i just land this one in v1-train ?

Because the follow up depends on: https://github.com/mozilla-b2g/gaia/commit/0f3a6c729085e322502c05b5dc932bf27ea0139c
Flags: needinfo?(felash)
(Assignee)

Updated

6 years ago
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
we need to ask approvals first ;)
Flags: needinfo?(felash)
You need to log in before you can comment on or make changes to this bug.