Refactor FXIOS-14620 [Autofill] Update AmEx card logo & add Discover dark mode#31840
Refactor FXIOS-14620 [Autofill] Update AmEx card logo & add Discover dark mode#31840lmarceau merged 4 commits intomozilla-mobile:mainfrom
Conversation
|
Are the before and after screenshots inverted? |
|
Ah ok I understand. We want the big Amex logo instead of the small one 🤔 I just want to double check with our PM first. Ill get back to you shortly. Thank you! |
|
Yeah the long "American Express" brand in full is being used mainly for the enterprise (but it does have different brand requirements to what's shown in the app anyways), whereas digital spaces and actual cards and/or their acceptance marks use predominantly this box logo. I wouldn't have made the call myself, so the linked ticket refers to when (& how) Desktop and Fenix added this newer "AmEx" logo for the same affordance. (No rush anywhere.) |
lmarceau
left a comment
There was a problem hiding this comment.
Product is ok with the change 👍 Approving! I ran the BR and will merge when green
|
Q: As I'm not expert in the PDF formats used in app bundling — I noticed (one of) the original vectors were a PDF 1.7 with "plain text" curves etc., and I see that what I got exported is an older simpler PDF 1.1 which however is not plain text, but appears binary, most likely compressed (which would explain the 90%+ file size saving on it) — not sure if there are any requirements for formats? (E.g. … would one format version rasterize better/easier than any other?) |
🧹 Tidy commitJust 5 file(s) touched. Thanks for keeping it clean and review-friendly! ✅ Per-file coverageAll changed files meet the threshold of 35.0%. Generated by 🚫 Danger Swift against f0646a7 |
|
@janbrasna where did you take the image from? Maybe we can ask our designers for guidance, just want to make sure I know what to ask for |
|
The originals come from the respective brand assets — the 512×512 square exports however are created with some postscript tools I have, that prefer simplicity and size — which I'm not sure is the right approach for the Xcode assets to be rasterized on the fly, so I'm checking in case there's some tribal knowledge what standards to use for xcassets imageset PDFs. (I basically took the existing PDFs as a blank slate, removed the contents, and juxtaposed the updated vectors onto the same canvas, hopefully keeping all the same prefs, profiles etc. for the change to be as minimal as possible.) |
|
I asked one of our designers for guidance, I'll let you know when I have an answer 🙏 Thank you |
|
So this is basically the difference: expand for sources [+]
(this one is the existing asset:) (this is the one I'm adding:) so it's apparently smaller, but the difference is in the "stream" binary content — judging by the Seeing none of the other assets use compressed streams/postscript, maybe I should look into converting the files from 1.1 to 1.7 and making sure it's all plaintext and no compression. It will be order of magnitude larger, but it perhaps performs better in target device rasterization? dunno |
|
@janbrasna do you happen to know what size the image should be ? |
|
"square"? ;D I'm not sure how intrinsic sizing works with PDFs, it does appear like 512 × 512px to me. Maybe I'll just go ahead and get rid of the optimization, and convert it to the same v1.7 plaintext as used elsewhere and never look back. It's not worth the ~25kB difference. EDIT: So with a little bit of tlc and flattening the PDFs I was actually able to convert to 1.7 uncompressed that are pretty much the same file size, so I'm fine with these I think… |
|
It's green ✅ Merging!Thank you Jan! |
📜 Tickets
Jira ticket FXIOS-14620 (+FXIOS-14619)
GitHub issue #31626 (+#31624)
💡 Description
AmEx brand update for consistency with desktop & mobile: #31626 (comment)
Discover contrast in dark UI: #31624
This also renames the Visa inverse dark mode logo for consistency.
🎥 Demos
📝 Checklist