Closed Bug 1035687 Opened 10 years ago Closed 10 years ago

[SMS] - check missing images

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: pivanov, Assigned: pivanov)

References

Details

Attachments

(3 files)

patch for Gaia/master
46 bytes, text/x-github-pull-request
azasypkin
: review+
Details | Review
corrupted@2x.png
5.17 KB, image/png
Details
Shot - Draft icon 12x12
37.72 KB, image/png
padamczyk
: ui-review+
Details 
We need to check following images:

apps/sms/style/icons/corrupted@2x.png
??? apps/sms/style/images/add_contact@2x.png
apps/sms/style/images/draft@2x.png
apps/sms/style/images/icons/attachments@2x.png
??? apps/sms/style/images/icons/close@2x.png
??? apps/sms/style/images/pattern@2x.png
??? apps/sms/style/images/pattern@2.25x.png
apps/sms/style/images/SMS_200x200_bubble@2.25x.png

??? - we need to double check this
Summary: [SMS] - remove unused images → [SMS] - check missing images
Attached file patch for Gaia/master — 
Hey Helen,
can you help me with this icon:
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/icons/corrupted.png
I need @2x version :)

ui-review? I compressed all images here


Thanks :)
Attachment #8462494 - Flags: ui-review?(hhuang)
Please see bug 1042747 attachment 8462364 [details] for the missing @2x assets.
nice :))) thanks

can we mark Bug 1042747 as duplicate of this bug?
Flags: needinfo?(azasypkin)
(In reply to Pavel Ivanov [:ivanovpavel] from comment #3)
> nice :))) thanks
> 
> can we mark Bug 1042747 as duplicate of this bug?

Sure, if in this bug you can add @2x versions for image #1, #2 (will be replaced soon anyway in bug 1026694) and #3 from bug 1042747 comment 0 + remove images #4 (with related style) and #5. #6 is still needed.
Flags: needinfo?(azasypkin)
Comment on attachment 8462494 [details] [review]
patch for Gaia/master

Hey Oleg, 
can you take a look?
Attachment #8462494 - Flags: feedback?(azasypkin)
Attached image corrupted@2x.png — 
Hi Pavel, the images has been updated. 
Please see the attached.
Comment on attachment 8462494 [details] [review]
patch for Gaia/master

Hey Pavel, just a few comments regarding to the patch:

1. attachments.png - became lighter after compression, but it's probably UX call, not mine.
2. draft.png - looks incorrectly on my flame;
4. clear.png - I guess we have to remove @1.5x, @2x, @2.25x too;
5. close.png - I think we should remove it too, or you see any usages in SMS app that I don't?

Also I don't see any size reductions in the following assets, why these assets are in the patch then?

1. actionicon_sms_send_30x30.png (all versions);
2. deliveredtick@2.25x.png;
3. exclamation@2.25x.png;
4. message_read.png (all versions);
5. report_delivered@1.5x.png;
6. report_exclamation.png (all versions).
Attachment #8462494 - Flags: feedback?(azasypkin)
Comment on attachment 8462494 [details] [review]
patch for Gaia/master

Hey Oleg,
I forgot to update my PR now I think everything is OK, can you check it again?

1. attachments.png - need to be white because of the SMS header color #00adad
2. draft.png - now it's OK 
4. clear.png - You are rignt. Done
5. close.png - You are rignt. Done

rest of the images are compressed that's why they are in the patch
Attachment #8462494 - Flags: feedback?(azasypkin)
(In reply to Pavel Ivanov [:ivanovpavel] from comment #9)
> Comment on attachment 8462494 [details] [review]
> patch for Gaia/master
> 
> Hey Oleg,
> I forgot to update my PR now I think everything is OK, can you check it
> again?
> 
> 1. attachments.png - need to be white because of the SMS header color #00adad
Sorry, I mean these "attachments.png" https://github.com/pivanov/gaia/blob/29984810ebbe22d65e08d51b1728620aac74463b/apps/sms/style/images/attachments%402x.png

> 
> rest of the images are compressed that's why they are in the patch
Yeah, that is my question, if they are compressed, why its size didn't change? :)
not sure ... maybe few bites are compressed on each image
I've compared images mentioned in comment 8 (#1-#6) and don't see even single byte win. Although it's not a problem, but I don't see any value either
Attachment #8462494 - Flags: review?(felash)
Comment on attachment 8462494 [details] [review]
patch for Gaia/master

Removing feedback request in favour of existing review request :)
Attachment #8462494 - Flags: feedback?(azasypkin)
Comment on attachment 8462494 [details] [review]
patch for Gaia/master

Redirecting to Oleg since he already spent some time on this and will be a better reviewer than me :)
Attachment #8462494 - Flags: review?(felash) → review?(azasypkin)
Comment on attachment 8462494 [details] [review]
patch for Gaia/master

Looks good, but I have two concerns that I left at GitHub.

Also I'm still not convinced that we need to update assets without any improvements (in size or quality), but if Julien is OK then I'm fine too :) Julien, what do you think? 

Thanks!
Flags: needinfo?(felash)
I agree with you, I don't see the point in updating the assets except the bubbles.
Flags: needinfo?(felash)
Comment on attachment 8462494 [details] [review]
patch for Gaia/master

Please, flip r? flag when you're ready!

Thanks!
Attachment #8462494 - Flags: review?(azasypkin)
Hey Patryk,

we need some help here the draft icon was 14x14 pixels but it was scaled to 10x10 till now. I make it 12x12 ans show it like 12x12 pixels is it OK or we nee to change the size?

Thanks :)


Oleg: Everything else is OK now
Flags: needinfo?(padamczyk)
Hey Pavel, "apps/sms/style/images/icons/attachments.png" were removed recently (bug 1026694).

Could you please remove it from your patch too?

Thanks!
Sure, sorry about this.

It's OK now
Flags: needinfo?(azasypkin)
Pavel if you scaled it to 12x12 via photoshop, it should be okay. Always good to show screenshots in the future.
Flags: needinfo?(padamczyk)
Attached image Shot - Draft icon 12x12 — 
Hey Patryk,

I forgot to attach a shot. Sorry about this.
Attachment #8465411 - Flags: ui-review?(padamczyk)
and Yes, I scaled it to 12x12 via PS
Attachment #8465411 - Flags: ui-review?(padamczyk) → ui-review+
Comment on attachment 8462494 [details] [review]
patch for Gaia/master

Looks good now, but please remove "apps/sms/style/images/icons/attachments@2.25x.png" file from your patch as well (+ tiny nit at github).

Thanks Pavel!
Attachment #8462494 - Flags: review+
Flags: needinfo?(azasypkin)
Thanks :)

Landed to master:
https://github.com/mozilla-b2g/gaia/commit/b1d3862fd9c1a1e4f157cd488630ac941f7aaadd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 8462494 [details] [review]
patch for Gaia/master

Clear flag seems this bug fixed.
Attachment #8462494 - Flags: ui-review?(hhuang)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: