Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elementor can't use the Customizr logo => let's implement the WP logo upload #1633

Closed
Nikeo opened this issue Nov 27, 2018 · 10 comments · Fixed by #1640

Comments

@Nikeo
Copy link
Contributor

commented Nov 27, 2018

Hi,

As shown in this recording, the logo in the Elementor header appears on other themes.
https://screencast-o-matic.com/watch/cFXO28rFwD

There is a conflict with Customizr that you cannot resolve, so please refund the $89.

Thanks,

Bill

Old logo option will be used if exists
in the customizer, only the WordPress logo upload setting will be available

=> this means that we drop support for svg and gif, for new logo upload. But this will still be allowed if old users don't update logo.

@Nikeo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

@eri-trabiccolo no rush on this. We'll discuss it

@Nikeo Nikeo added the dev-scheduled label Nov 28, 2018

@Nikeo Nikeo changed the title Elementor can't use the Customizr logo Elementor can't use the Customizr logo => let's implement the WP logo upload Nov 28, 2018

@eri-trabiccolo

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

In our chat we decided to:

  1. display the current logo option choice if set, and no wp logo set, in front
  2. remove the current customizer logo control
  3. display only the wp logo control

This, while enough in most of the cases, doesn't cover the case when an old user having a logo set decides to not use it anymore.

So if we want to stick on what we decided, we can propose, as FAQ for example, an hack to reset the old logo which would be:
"Set a new logo and then remove it, then publish your changes"

Then we just need a way to differentiate a wp logo never set from a wp logo not set (matter of defaults).

@Nikeo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

Yes I'm OK with this solution : we clear the old logo option when the new wp logo is set. This way, setting a new logo and removing it could do the trick.
The clearing of the old logo could be done on do_action( "customize_save_{$id_base}", $this );
see https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp-customize-setting.php#L522

@eri-trabiccolo

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

Ah yeah makes sense, thanks for the tip!

@eri-trabiccolo

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

I'm not sure clearing the old logo is something we should do... you know I'm always frightened when is about manually remove theme options :)

Anyway a PR is on the way.
Consider that removing the old logo uploader we also remove the customizer LOGO "subsection" title, and the logo control itself will sit at the very top of the Site Identity section. We might want to improve the ordering of that section with js?!

@eri-trabiccolo

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

Also, the SVG upload is still allowed, but the cropping need to be skipped.

eri-trabiccolo added a commit to eri-trabiccolo/customizr that referenced this issue Dec 5, 2018

@eri-trabiccolo eri-trabiccolo added the fixed label Dec 5, 2018

@Nikeo Nikeo closed this in #1640 Dec 14, 2018

@Nikeo Nikeo reopened this Dec 15, 2018

@Nikeo

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2018

@eri-trabiccolo I'm releasing Customizr now but there's an issue with the new WP logo.
How to reproduce :

  1. Set a logo on the previous version of customizr
  2. Update Customizr

On the front end, everything's fine, the logo is displayed. But when customizing, the logo is not displayed and replaced by the site title.

I might be wrong but I don't remember that we validated this behaviour when discussing the spec. Can you fix this next week ?
Thanks, have a good week end !

@Nikeo Nikeo removed the fixed label Dec 15, 2018

@eri-trabiccolo

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

Hi Nico,
nope, you're right, that's not something we've validated and it's certainly a bug.
I'm sorry but I haven't noticed this, not sure why.
I'm investigating into this right now.

@eri-trabiccolo

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

Looks like the problem is that the the theme option wp_logo even if not is not null when customizing, but rather an empty string.

I'm doing some tests to see whether we can safely change our check without any relevant side effect.

@eri-trabiccolo

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

Ok, seems fine to me.
I wanted to use the "null" value because of my idea to emulate the "isset", but we can go with the less strict "falsy" as we clear the old logo on customize_save_custom_logo as per your suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.