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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tadityar’s picture

tadityar CreditAttribution: tadityar commented
Assigned: Unassigned » tadityar
tadityar’s picture

tadityar CreditAttribution: tadityar commented

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

tadityar’s picture

tadityar CreditAttribution: tadityar commented
FileSize
seven-screenshot-update-2.patch68.36 KB
Configuration | d8.png55.72 KB
tadityar’s picture

tadityar CreditAttribution: tadityar commented
Status: Active » Needs review

Change status

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Needs work
FileSize
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 CreditAttribution: 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
CreditAttribution: LewisNyman commented

Yeah I think it's the incorrect size right now

tadityar’s picture

tadityar CreditAttribution: tadityar commented

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

tadityar’s picture

tadityar CreditAttribution: tadityar commented
FileSize
update-seven-screenshot-2389745-9.patch70.43 KB
tadityar’s picture

tadityar CreditAttribution: tadityar commented
Status: Needs work » Needs review

status change

tadityar’s picture

tadityar CreditAttribution: tadityar commented
FileSize
update-seven-screenshot-2389745-11.patch70.26 KB

wrong size uploaded, update to new size

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Needs work
FileSize
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 CreditAttribution: tadityar commented
FileSize
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 CreditAttribution: 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
CreditAttribution: LewisNyman commented

It's ok to have trailing white space in images

tadityar’s picture

tadityar CreditAttribution: tadityar commented
Assigned: tadityar » Unassigned
jedihe’s picture

jedihe CreditAttribution: jedihe commented
Assigned: Unassigned » jedihe
hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Needs work » Needs review
FileSize
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 CreditAttribution: tadityar commented
Status: Needs review » Reviewed & tested by the community
FileSize
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 CreditAttribution: 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 🇨🇦
CreditAttribution: webchick 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.