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 :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Hm. Does it make sense to do this with jQuery and not in the template itself? I imagine other themes would have this bug otherwise?

tim.plunkett’s picture

Status: Needs work » Needs review

Unless I'm mistaken, there IS no template for previews, which is why I figured Jeff resorted to JS.

Jeff Burnz’s picture

There is a template but its pure HTML (preview.html), all the logic for it is in preview.js.

The logic for the logo is:

// Change the logo to be the real one.
      if (!this.logoChanged) {
        $('#preview #preview-logo img').attr('src', Drupal.settings.color.logo);
        this.logoChanged = true;
      }

Drupal.settings.color.logo can be null, which is what the patch takes advantage of to completely remove the elements.

webchick’s picture

This 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?

Jeff Burnz’s picture

I don't know, I'll dig deeper and report back.

Jeff Burnz’s picture

I 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.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

It 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.

jensimmons’s picture

+1

Jeff Burnz’s picture

Yep, +1 to #8, commit this and reopen for D8 so we can re-evaluate how this whole preview feature should work.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

No 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.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.45 KB

As 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.

jensimmons’s picture

I just tested this.... works fine. (Be sure to clear the cache.)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, thanks for checking into it.

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

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