Problem/Motivation

The Seven theme looks a little different now, we should update the screenshot.

Proposed resolution

Update the screenshot

Remaining tasks

Update the screenshot

User interface changes

A better image of seven

API changes

None

CommentFileSizeAuthor
#19 Screen Shot 2014-12-25 at 5.55.09 PM.png504.19 KBtadityar
#18 update-seven-screenshot-2389745-18.patch25.95 KBhussainweb
#13 Screen Shot 2014-12-12 at 8.23.36 PM.png532.1 KBtadityar
#12 Screen Shot 2014-12-12 at 09.49.15.jpg788.18 KBlewisnyman
#11 update-seven-screenshot-2389745-11.patch70.26 KBtadityar
#9 update-seven-screenshot-2389745-9.patch70.43 KBtadityar
#5 Screenshot 2014-12-09 23.35.28.jpg131.39 KBlewisnyman
#3 Configuration | d8.png55.72 KBtadityar
#3 seven-screenshot-update-2.patch68.36 KBtadityar

Comments

tadityar’s picture

tadityar commented
Assigned: Unassigned » tadityar
tadityar’s picture

tadityar commented

@LewisNyman
Actually, which screens are updated? I can't seem to notice the difference..

tadityar’s picture

tadityar commented
StatusFileSize
new seven-screenshot-update-2.patch68.36 KB
new Configuration | d8.png55.72 KB
tadityar’s picture

tadityar commented
Status: Active » Needs review

Change status

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work
StatusFileSize
new Screenshot 2014-12-09 23.35.28.jpg131.39 KB

Thanks for the patch, for some reason the image in the patch seems to be corrupted? When I downloaded the screenshot it seemed fine.

I think there's a special option in git diff for binary files? Maybe try the patch with git diff --binary.

Also on small thing, I noticed that the image is actually a little bit smaller than the size we display it at, any chance we can up the size of the screenshot to 300px?

tadityar’s picture

tadityar commented

@LewisNyman I'll try git diff --binary. But when I opened the previous screenshot in PS the dimension was 294x219. Should I just increase it to 300?

lewisnyman’s picture

lewisnyman
he/him
commented

Yeah I think it's the incorrect size right now

tadityar’s picture

tadityar commented

@LewisNyman it still doesn't work with the binary turned on.. I wonder how

tadityar’s picture

tadityar commented
StatusFileSize
new update-seven-screenshot-2389745-9.patch70.43 KB
tadityar’s picture

tadityar commented
Status: Needs work » Needs review

status change

tadityar’s picture

tadityar commented
StatusFileSize
new update-seven-screenshot-2389745-11.patch70.26 KB

wrong size uploaded, update to new size

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work
StatusFileSize
new Screen Shot 2014-12-12 at 09.49.15.jpg788.18 KB

Yeah! This now seems to be working correctly, but the image is just one pixel smaller than the display. I am being very picky but I think when a browser has to upscale an image even by a small amount it compromises the quality.

tadityar’s picture

tadityar commented
StatusFileSize
new Screen Shot 2014-12-12 at 8.23.36 PM.png532.1 KB

@LewisNyman hmm.. it's weird, when I check the dimension of the image it says 300x225

ss

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented

I confirm the patch needs work,

1) While applying with -v argument I get the below warning message 

update-seven-screenshot-2389745-11.patch:121: trailing whitespace.
                                                                                                    
update-seven-screenshot-2389745-11.patch:122: trailing whitespace.
                                                                                                    
update-seven-screenshot-2389745-11.patch:123: trailing whitespace.
                                                                                                    
update-seven-screenshot-2389745-11.patch:124: trailing whitespace.
                                                                                                    
update-seven-screenshot-2389745-11.patch:125: trailing whitespace.
                                                                                                    
Checking patch core/themes/seven/screenshot.png...
Applied patch core/themes/seven/screenshot.png cleanly.
warning: squelched 115 whitespace errors
warning: 120 lines add whitespace errors.

2) I think, the expected size of snapshot is 294x219 (same as bartik). Indeed, it looks relatively distorted now.

lewisnyman’s picture

lewisnyman
he/him
commented

It's ok to have trailing white space in images

tadityar’s picture

tadityar commented
Assigned: tadityar » Unassigned
jedihe’s picture

jedihe commented
Assigned: Unassigned » jedihe
hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new update-seven-screenshot-2389745-18.patch25.95 KB

I am reading the comments here. I think the image size is still supposed to be 294x219. The size 300x225 as seen in screenshots is actually the size of the entire box with margins and padding. That is the reason there is a discrepancy seen in #12. In that case, the file is actually scaled down to 294px, which makes the image size 294x220 and with margins and padding, it becomes 300x226.

I am updating the patch with the image from #3. Plus, I have optimized it with tinypng, which is lossy, but brings the image to the same size as the current screenshot.png. Lossless optimization brings it to 36kb which is almost thrice the size. I think we should be fine with slightly lossy compression for the benefits on size.

tadityar’s picture

tadityar commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screen Shot 2014-12-25 at 5.55.09 PM.png504.19 KB

Tested on simplytest.me . The patch applies and the size of the screenshot is also 294x219 (the same as other screenshot) and I think it's fine with the compression as people won't try to read it anyway. So RTBC for me.
ss

jedihe’s picture

jedihe commented
Assigned: jedihe » Unassigned

Returning to unassigned... simplytest.me is not working for me + I haven't set up a proper local env for D8.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 3509718 on 8.0.x 
    Issue #2389745 by tadityar, LewisNyman, hussainweb: Update Seven's...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.