Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
If the user toggles the logo off in the theme settings the logo is not removed from the preview - instead the logo path is returned as null and alt text shows - not good.
Needs a bit of js added to preview.js to test if the relevant setting is null and remove the #preview-logo div if so. If you have a better way of doing this please do :)
Comment | File | Size | Author |
---|---|---|---|
#12 | drupal-bartik-987102-12.patch | 1.45 KB | tim.plunkett |
bartik-preview-logo.patch | 1.42 KB | Jeff Burnz | |
Comments
Comment #1
tim.plunkettI came across this during #844734: Bartik, do not import style.css from preview.css but completely forgot about it.
It also mirrors the changes made in #972136: Site name alignment wrong with no logo.
Comment #2
webchickHm. Does it make sense to do this with jQuery and not in the template itself? I imagine other themes would have this bug otherwise?
Comment #3
tim.plunkettUnless I'm mistaken, there IS no template for previews, which is why I figured Jeff resorted to JS.
Comment #4
Jeff Burnz CreditAttribution: Jeff Burnz commentedThere is a template but its pure HTML (preview.html), all the logic for it is in preview.js.
The logic for the logo is:
Drupal.settings.color.logo can be null, which is what the patch takes advantage of to completely remove the elements.
Comment #5
webchickThis still feels like it's in the wrong place, because all themes are going to want this to happen. Hm.
Is there a way for Colour module to supply this logic?
Comment #6
Jeff Burnz CreditAttribution: Jeff Burnz commentedI don't know, I'll dig deeper and report back.
Comment #7
Jeff Burnz CreditAttribution: Jeff Burnz commentedI think the only way to fix this properly is to make preview.html into preview.tpl.php - i.e. a proper template with process functions and so on, which would make this D8 material. I don't think we have to many choices for D7, if someone else has a suggestion I'd be happy to see it, right now I have very little time available for core dev work, at least not enough time before D7 is released.
Comment #8
tim.plunkettIt should be noted that Garland doesn't show the logo in it's preview, in D6 or D7, perhaps for this reason. So as far as core themes go, a fix in color.module would still only benefit Bartik.
After looking through color.module again, Jeff is right. There is no trivial way to fix this, because color.module expects a hardcoded preview.html file. It can't even be done globally in modules/color/preview.js since themes can now provide their own preview.js (see #776684: The color.module preview HTML is hardcoded).
I think this should be committed as is, then this can be reopened and pushed to D8, or I can open up a separate follow-up issue for D8.
Comment #9
jensimmons CreditAttribution: jensimmons commented+1
Comment #10
Jeff Burnz CreditAttribution: Jeff Burnz commentedYep, +1 to #8, commit this and reopen for D8 so we can re-evaluate how this whole preview feature should work.
Comment #11
webchickNo longer applies; needs a re-roll.
I'm still worried that this is a one-off fix, e.g. I imagine Garland has the same issue. But I don't understand the underlying code well enough to explain how it could be fixed differently.
Comment #12
tim.plunkettAs I said in #8, Garland doesn't display its logo, so it wouldn't benefit in the short-term anyway, and the long-term fix is way too much for D7.
Just had to change IDs after #988184: Duplicate ID's can mess up the preview, back to RTBC.
Comment #13
jensimmons CreditAttribution: jensimmons commentedI just tested this.... works fine. (Be sure to clear the cache.)
Comment #14
webchickOk, thanks for checking into it.
Committed to HEAD. Thanks!