WHEN
1- i created a new theme with no screenshot
2- i enabled the new theme from admin/appearance

SOLUTION
is simply to reduce the padding for the themes with no screen in the enabled-listing area by adding the following to system.admin.css line 269
body:not(.toolbar-vertical) .system-themes-list-enabled .no-screenshot {
padding-bottom:18%;
}

CommentFileSizeAuthor
#24 applied-patch.png23.23 KBalexrayu
#22 issue-2075475_no_screenshot-22.patch3.7 KBsqndr
#22 issue-2075475_no_screenshot2.png44.96 KBsqndr
#22 issue-2075475_no_screenshot1.png42.17 KBsqndr
#22 core-theme-no-screenshot-2075475-14.patch4.38 KBsqndr
#19 no_screenshot.png731 bytestemaruk
#17 no-screenshot-2.png680.82 KBsqndr
#15 2075475.default_screenshot.patch2 KBoostie
#14 core-theme-no-screenshot-2075475-14.patch4.38 KBtemaruk
#13 Screen Shot 2014-03-24 at 13.47.43.png399.43 KBsqndr
#13 Screen Shot 2014-03-24 at 13.47.29.png571.96 KBsqndr
#13 Screen Shot 2014-03-24 at 13.47.26.png517.79 KBsqndr
#13 no_screenshot-issue-2075475.patch761 bytessqndr
#9 fix-toolbar-top.png312.76 KBsqndr
#9 fix-toolbar-bottom.png200.01 KBsqndr
#4 2075475-themes_with_no_screenshot_css_height_issue-4.patch640 bytesoostie
#3 2075475-themes_with_no_screenshot_css_height_issue-3.patch639 bytesmeladawy
#1 theme-system_themes-with-no-screen-CSS-heigh-issue_2075475_1.patch538 bytesmeladawy
theme_system_css_issue.png311.3 KBmeladawy

Comments

meladawy’s picture

meladawy commented
Status: Active » Needs review
StatusFileSize
new theme-system_themes-with-no-screen-CSS-heigh-issue_2075475_1.patch538 bytes

Status: Needs review » Needs work

The last submitted patch, theme-system_themes-with-no-screen-CSS-heigh-issue_2075475_1.patch, failed testing.

meladawy’s picture

meladawy commented
Status: Needs work » Needs review
StatusFileSize
new 2075475-themes_with_no_screenshot_css_height_issue-3.patch639 bytes
oostie’s picture

oostie
Primary language Dutch
Location Kampen
commented
StatusFileSize
new 2075475-themes_with_no_screenshot_css_height_issue-4.patch640 bytes

I was able to apply this patch and can confirm that it solves this issue.
I deleted a whitespace on the padding-bottom rule and created a new patch.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Component: theme system » Seven theme
Issue summary: View changes
Issue tags: +CSS

I gave this a test and it still giving huge height image. It touches it up a bit for when the toolbar is up but still large when it's down.

This likely needs another div for the padding's percentage to pickup the correct container's width:
http://wellcaffeinated.net/articles/2012/12/10/very-simple-css-only-prop...

Or maybe slightly better without the extra div.
http://www.mademyday.de/css-height-equals-width-with-pure-css.html

Also the correct proportions should probably be 74% to match the other image sizes.

Thanks for creating this issue and patching!

lewisnyman’s picture

lewisnyman
he/him
commented
Component: Seven theme » CSS

Yeah it looks like we need to change the proportion to match. It looks like .no-screenshot also has a max-width set on it. We should try removing it.

I don't understand the .toolbar-vertical selector inclusion? How is the toolbar involved here?

ps. If we are putting this in system.admin.css then we should probably assign this to CSS instead of the Seven theme right?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

That toolbar stuff is baffling to me as well, I'd suggest removing it but maybe need to do a git bisect to see what issue it was introduced in.

And yes to the component ps, that makes way more sense, thanks:)

sqndr’s picture

sqndr commented
Assigned: Unassigned » sqndr
sqndr’s picture

sqndr commented
StatusFileSize
new fix-toolbar-bottom.png200.01 KB
new fix-toolbar-top.png312.76 KB

The best way to solve might be to use a placeholder image. This way, all the div have an actual image which should then fix this issue. Any thoughts?

Also, the current patch only fixes the problem when the toolbar is on the top of the page. If I moved the toolbar to the right, the problem occurred again:

Toolbar is on the top, patch looks good:

Patch does not apply when the toolbar is on the left. Also - adding the same padding-bottom does not result in the correct results.

sqndr’s picture

sqndr commented
Assigned: sqndr » Unassigned
oostie’s picture

oostie
Primary language Dutch
Location Kampen
commented

I found out that the same occurs with disabled theme's.
The DIV without an image is slightly smaller then the one's with an image.

lewisnyman’s picture

lewisnyman
he/him
commented

Yes let's try and add a default image instead of text, then it will be easy to make it consistent.

sqndr’s picture

sqndr commented
StatusFileSize
new no_screenshot-issue-2075475.patch761 bytes
new Screen Shot 2014-03-24 at 13.47.26.png517.79 KB
new Screen Shot 2014-03-24 at 13.47.29.png571.96 KB
new Screen Shot 2014-03-24 at 13.47.43.png399.43 KB

The other thing to do, might be to include a fixed height for the .toolbar-vertical .no-screenshot. We could do this, because by default, this div is set to display:none. The empty 'screenshot' (so, this no-screenshot) is only shown when the min-width is 45em: @media screen and (min-width: 45em) {

Some screenshots are attached, showing what the patch does.

So - this might be another way to fix the issue.

temaruk’s picture

temaruk commented
StatusFileSize
new core-theme-no-screenshot-2075475-14.patch4.38 KB

Implemented the default image solution, as mentioned in #2075475-12: Theme with no screenshot CSS heigh issue . Attached a patch, applies against *current* 8.x HEAD.

As you will see, I have removed some CSS styling that became unnecessary this way. Also, removed the else branch from the system-themes-page.html.twig template file, which take care of rendering the "no-screenshot" case, and moved this decision into the relevant preprocess function (template_preprocess_system_themes_page), as I consider this logic more appropriate in that layer of the process.

oostie’s picture

oostie
Primary language Dutch
Location Kampen
commented
StatusFileSize
new 2075475.default_screenshot.patch2 KB

Here's a patch which will provide a default image, i'm not that good with Photoshop so if somebody else can create the image :)

oostie’s picture

oostie
Primary language Dutch
Location Kampen
commented

Damn temaruk, just a minute faster then me ;-)

sqndr’s picture

sqndr commented
StatusFileSize
new no-screenshot-1.png622.04 KB
new no-screenshot-2.png680.82 KB

I've tested the patch from #14 by creating a new theme without a screenshot. Seems like this solves the issue, since there now is an actual image.


lewisnyman’s picture

lewisnyman
he/him
commented

This looks good. I really like this. One question about the image, what font is it? It would be nice to match Lucida Grande, which is used by Seven.

temaruk’s picture

temaruk commented
StatusFileSize
new no_screenshot.png731 bytes

Attached a new version of the image, with Lucida Grande. (The previous was Helvetica Neue.)

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work

Great, it looks like someone needs to roll this into a patch?

sqndr’s picture

sqndr commented
Assigned: Unassigned » sqndr
sqndr’s picture

sqndr commented
StatusFileSize
new core-theme-no-screenshot-2075475-14.patch4.38 KB
new issue-2075475_no_screenshot1.png42.17 KB
new issue-2075475_no_screenshot2.png44.96 KB
new issue-2075475_no_screenshot-22.patch3.7 KB

Right, I added the new image with the Lucida Grande font from #19 to the patch from #14. Needs a review.

sqndr’s picture

sqndr commented
Status: Needs work » Needs review

Needs a review.

alexrayu’s picture

alexrayu commented
StatusFileSize
new applied-patch.png23.23 KB

Works for me.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Reviewed & tested by the community

Woop! I'm happy with this.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Sorry, can someone direct me as to which patch needs committing? I'm a little confused. Looks like a great fix, though!

lewisnyman’s picture

lewisnyman
he/him
commented

Hey, I forgot to mention that issue-2075475_no_screenshot-22.patch is the latest patch. We discussed it in person and sqndr accidentally posted the original #14 patch again by mistake

webchick’s picture

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

Duh, right. That makes sense. Sorry for being an eeeediot. :)

Committed and pushed to 8.x. Thanks!

  • Commit f717159 on 8.x by webchick: 
    Issue #2075475 by sqndr, Oostie, meladawy, temaruk, LewisNyman: Theme...
sqndr’s picture

sqndr commented
Assigned: sqndr » Unassigned

Status: Fixed » Closed (fixed)

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