Opened 9 years ago
Closed 9 years ago
#33755 closed task (blessed) (fixed)
Add Custom Logo to WordPress Core
Reported by: | fatmedia | Owned by: | obenland |
---|---|---|---|
Milestone: | 4.5 | Priority: | normal |
Severity: | normal | Version: | 4.6 |
Component: | Customize | Keywords: | has-screenshots |
Focuses: | ui | Cc: |
Description (last modified by )
Now the the Site Icon feature is bundled in core, I think it makes perfect sense to include the Custom Logo feature as well. It's already part of Jetpack, just like the Site Icon feature was previously, and it's an important aspect of almost every modern website.
With this not being part of core, theme authors are forced to either roll their own, add support for the feature in Jetpack, or do a combination of the above by rolling their own which falls back to the Jetpack module if it's active.
IMO this is a pretty lousy situation which could be fixed by improving the code in the Jetpack module and rolling into core as a new feature.
Attachments (21)
Change History (174)
#2
in reply to:
↑ 1
;
follow-up:
↓ 4
@
9 years ago
Replying to markkaplun:
-1
Placing the logo requires CSS changes, and it is actually faster and more future proof to have the logo image as part of the CSs.
So if you want to properly implement such a feature you need to eliminate the need to do any CSS changes and let the user drag and drop the logo to its proper location to get it proper location. Then of course the question is how do you handle cases like positioning the logo in the middle of the width of the site, something that is actually much easier to do with CSS then manually.
And then you also need to supprot a different logo for mobile.
Not really against it, just thinking that value for money is very low in this case. Jetpack has it for feature parity with wordpress.com but in wordpress.com most people can not edit their CSS which is not true for stand alone wordpress.
I don't disagree with you. In a perfect world, everyone would know how to add their logo using CSS. Unfortunately, most users have no idea how to deal with this. I imagine that's why the feature was added to both WordPress.com and Jetpack in the first place.
Whether core decides to add this or not, theme authors need a way to allow users to easily upload a logo. Currently, a large number of free and commercial themes are using one of the methods I mentioned in the ticket. These implementations vary greatly in code quality and user experience, so it seems like a really good place for core to step in and provide a standard.
If the feature is tied to theme support, I don't really see the potential harm in adding it. It will create a unified experience for anyone who needs to include a logo uploader for their users. Those who don't can continue doing it however they have in the past.
#3
@
9 years ago
Something like:
add_theme_support( 'site-logo' ); set_logo_size( 600, 200 );
Base it on post thumbnails?
#4
in reply to:
↑ 2
@
9 years ago
Replying to fatmedia:
I don't disagree with you. In a perfect world, everyone would know how to add their logo using CSS. Unfortunately, most users have no idea how to deal with this. I imagine that's why the feature was added to both WordPress.com and Jetpack in the first place.
No one should know CSS in order to have a wordpress site, but people that pay for graphical designer to generate a logo for them will most likely pay as well for a web developer to place it, and the web developer hopefully knows css. So this is really a feature useful mainly for theme developers.
Do you think that you can convince the theme review team that it a recommended feature for the themes in the repository?
If the feature is tied to theme support, I don't really see the potential harm in adding it. It will create a unified experience for anyone who needs to include a logo uploader for their users. Those who don't can continue doing it however they have in the past.
All code contains bugs, the more code you have the harder is to maintain it and because of backward compatibility it is hard to remove code once it is in.
#5
@
9 years ago
- Milestone Awaiting Review deleted
- Resolution set to duplicate
- Status changed from new to closed
Duplicate of #33597.
#6
@
9 years ago
- Resolution duplicate deleted
- Status changed from closed to reopened
This doesn't really seem like a duplicate of #33597 to me. Re-opening for reconsideration.
#7
@
9 years ago
- Keywords needs-patch ux-feedback added
- Milestone set to Awaiting Review
Jetpack has a site logo feature which we might be able to use as inspiration.
#8
@
9 years ago
I'm keen to work on this, but this would be my first core contribution so I'm not 100% sure where to start. According to the 4.3 post-mortem, it seems like this should probably be built out as a feature plugin.
Are there any documented procedures for setting up a feature plugin? Should there be discussions about what should be included before a repo is actually set up? If so, where's the best place to do that? Slack? Here?
Any help from more experienced contributors would be greatly appreciated.
#9
follow-up:
↓ 10
@
9 years ago
Most logos these days are SVG's (or at least thats where things are heading) and seeing how core does not support uploading SVG's by default I cannot see how this feature should be considered for core.
#10
in reply to:
↑ 9
@
9 years ago
Replying to paulwilde:
Most logos these days are SVG's (or at least thats where things are heading) and seeing how core does not support uploading SVG's by default I cannot see how this feature should be considered for core.
There has been some movement recently on getting SVG support in core (#24251).
That aside, I don't think we can really gauge whether or not most logos are SVGs right now. SVG support would be great, but this feature stands on its own with or without it IMO.
#12
@
9 years ago
My initial reaction is a vote to keep this in JetPack or other plugins that implement this rather than have it as a core feature. However, I think some fact finding and research would be good as to if this would be a popular feature for core. I have a feeling it isn't going to be as popular as maybe initially thought. That could be a wrong notion, so be great to figure out.
#13
follow-up:
↓ 14
@
9 years ago
I think there's enough of a demand, and enough self-made implementations of this that including site logos as something themes could support is a really good idea. It's a common feature (most business sites will have a logo, at the very least) and right now we leave it up to theme authors to come up with or support what they think is the best implementation, which can have negative UX implications. I think it would benefit the overall theme ecosystem to make it a standard feature.
#14
in reply to:
↑ 13
@
9 years ago
Replying to melchoyce:
I think there's enough of a demand, and enough self-made implementations of this that including site logos as something themes could support is a really good idea. It's a common feature (most business sites will have a logo, at the very least) and right now we leave it up to theme authors to come up with or support what they think is the best implementation, which can have negative UX implications. I think it would benefit the overall theme ecosystem to make it a standard feature.
Agreed.
#16
follow-up:
↓ 17
@
9 years ago
I'd like to see a clear explanation of why core needs to have a site logo and a site icon, and how we would present that distinction for users before supporting this personally.
Most of the users I work with (communications/journalism students) already don't understand why they need a site title and a tagline; I don't know that having site icons and logos is justifiably needed for 80%, or even a majority, of sites. Perhaps if we can find a way to expand the site icon feature to support different versions of an image (ie, non-square variants as opposed to a separate option) we could satisfy more complex needs while keeping things simple for the typical user.
#17
in reply to:
↑ 16
@
9 years ago
Replying to celloexpressions:
I'd like to see a clear explanation of why core needs to have a site logo and a site icon, and how we would present that distinction for users before supporting this personally.
Most of the users I work with (communications/journalism students) already don't understand why they need a site title and a tagline; I don't know that having site icons and logos is justifiably needed for 80%, or even a majority, of sites. Perhaps if we can find a way to expand the site icon feature to support different versions of an image (ie, non-square variants as opposed to a separate option) we could satisfy more complex needs while keeping things simple for the typical user.
IMO the reasoning behind the need for this feature has already been explained pretty well, but I do agree that having both a site "icon" and a "logo" could be both pointless and maybe even a bit confusing in some situations.
I don't think a logo and an icon can be considered the same thing as they have very different purposes; however, changing the site icon into a more catch-all "site branding" module might be more reasonable than adding a completely separate logo component.
#19
@
9 years ago
This is just a playing with what Site Identity would look like as a full on branding section. Things that would need testing and exploration are:
- If it's all site identity or site branding.
- Hierarchy of elements.
- If this even works for users. Is it too much in one long section?
Note: I've only done a placeholder for the site logo, rather than a full design.
I do think it has merits if only to test and see as an option. If we do add more settings having a section for them all does make sense.
#20
@
9 years ago
Looks good for me. I would put "Site Logo" above "Site Icon" (but below "Site Title" and "Tagline"), though.
#21
@
9 years ago
With regards to order, I think it would be good to work out what users want first. Once that's set we can then iterate and reorder.
#22
follow-up:
↓ 23
@
9 years ago
Having both "Site Icon" and "Site Logo" is really confusing to me, and likely others. I really want to see user tests (and a feature plugin) before we consider it. That said, if we want to go this route we shouldn't call it "Site Logo," we should call it "Logo."
#23
in reply to:
↑ 22
@
9 years ago
Replying to samuelsidler:
Having both "Site Icon" and "Site Logo" is really confusing to me, and likely others. I really want to see user tests (and a feature plugin) before we consider it. That said, if we want to go this route we shouldn't call it "Site Logo," we should call it "Logo."
+1 for both "Logo" and a round of user tests to make sure there's not too much confusion, and make sure our layout is good. otherwise, I like this.
#24
follow-up:
↓ 29
@
9 years ago
- Keywords has-patch needs-testing added; needs-patch removed
Thanks for the patch, @obenland!
I chatted with him about this a bit, and he notes:
- This needs unit tests and docs
- Has a workaround for
header_text
option for themes that support logos but don’t support custom headers. Jetpack uses its own option for this, but we'd ideally want to do this differently for core.
It will also (as noted previously in the ticket) need at least a round of user tests.
In my testing of 33755.diff, noticed the following:
- It's very hard to test, since it needs a theme that both supports site logos, and is using
the_site_logo()
rather thanjetpack_the_site_logo()
. @karmatosed is checking into what it would take to have a twentysixteen to test. - I'd put "logo" above "site icon", since once a site icon is inserted, at least at mid-height resolutions, "logo" gets hidden entirely because icons are always a square aspect ratio.
#25
follow-up:
↓ 26
@
9 years ago
For what it's worth, if anyone has a better idea of naming for this, please suggest it -- we used to get all sorts of confusion from Jetpack users trying to differentiate between "Site Logo" (this) and "Site Icon" (favicon).
#26
in reply to:
↑ 25
@
9 years ago
Replying to georgestephanis:
For what it's worth, if anyone has a better idea of naming for this, please suggest it -- we used to get all sorts of confusion from Jetpack users trying to differentiate between "Site Logo" (this) and "Site Icon" (favicon).
In the patch it's currently "Logo".
#27
@
9 years ago
Nice work @obenland! This is a fun feature; I think 'Logo' or 'Site Logo' works for the naming.
I tested this out with a few themes that were setup to support Jetpack site logos, modifying the main call to use the new core function as a fallback. The logo support worked great, and I could then switch between themes supporting site logos and retain the same logo.
#28
@
9 years ago
- remove a console.log statement;
- change priority of logo section, placing it above site icon section
- diff: https://cloudup.com/cfOnwOxwCSU
#29
in reply to:
↑ 24
@
9 years ago
- Keywords needs-unit-tests added
Replying to mikeschroder:
- I'd put "logo" above "site icon", since once a site icon is inserted, at least at mid-height resolutions, "logo" gets hidden entirely because icons are always a square aspect ratio.
Works for me.
Replying to adamsilverstein :
remove a console.log statement;
Good catch! That reminds me, when this gets in, @westonruter deserves props too.
#31
@
9 years ago
- Milestone changed from Awaiting Review to 4.5
Let's consider this for 4.5, if user tests look good.
#32
@
9 years ago
Another good reason to put “Logo” above “Site Icon” is that it stays closer to “Site Title” — which makes sense because the logo and the title usually sit together on the site pages.
To avoid confusion, it might be a good idea to add a brief explanation below “Logo” (as in below “Site Icon”). Something like “The Logo appears on the pages of your site. It can be used in place of the Site Title and Tagline.”
#33
@
9 years ago
There is a branch of Twenty Sixteen that can be used to user test this here: https://github.com/wordpress/twentysixteen/tree/site-logo. @mikeschroder feel free to pass this onto user tests now. Big thanks to @iamtakashi for working with me on this branch.
#34
follow-ups:
↓ 35
↓ 43
@
9 years ago
Feature looks good. I agree Logo should appear above Site Title & Tagline.
I think there will definitely be confusion between Site Logos and Site Icons, and part of that isn't that we're adding logos, but that the UI for Site Icons is confusing. You're never really going to see a big, 512x512 image for your Site Icon anywhere — displayed inside the Customizer panel like that, it lacks all context. I think we should bring the preview UI from inside the Media Modal into the panel itself. Attaching a design to demonstrate what I mean.
#35
in reply to:
↑ 34
@
9 years ago
Replying to melchoyce:
Feature looks good. I agree Logo should appear above Site Title & Tagline.
I think there will definitely be confusion between Site Logos and Site Icons, and part of that isn't that we're adding logos, but that the UI for Site Icons is confusing. You're never really going to see a big, 512x512 image for your Site Icon anywhere — displayed inside the Customizer panel like that, it lacks all context. I think we should bring the preview UI from inside the Media Modal into the panel itself. Attaching a design to demonstrate what I mean.
+100 I agree that the way the Site Icon is presented is a big part of the confusion. It's basically a favicon/bookmark icon and having it presented that way makes it much clearer.
This latest mock-up looks a lot closer to the reality for the majority of sites using these features. I agree user testing will help, but I think this is getting pretty close.
I'm really excited to see progress on this. I think both theme authors and site owners will really appreciate it.
#36
follow-up:
↓ 37
@
9 years ago
This is a cool feature, but I'm still not convinced that this is core material. My primary question remains, for how many users will there be a need for a site logo that is distinct from a site icon (favicon)?
In addition to user testing to determine whether users can make sense of the distinction between the two features in the UI, we need to confirm that enough sites would have a logo that is different from their icon. For many sites, even coming up with something to use as the icon is challenging (but necessary, to convey a complete site identity). The sites where the owner (company or individual) has the resources to create separate graphics that could be used for an icon vs. a logo are also likely the sites that would be able to jump through the hoop of installing a plugin to activate this functionality. For those with more limited resources, seeing both a logo and an icon option that seem to need to be set could be discouraging.
One possible approach to mitigate these types of issues would be to use the site logo as a fallback for the site icon if one isn't set (as that seems like a much more important option that shouldn't ever be empty if possible). Obviously not ideal, but likely better than nothing when some users set a logo an no icon, as I'm sure a certain percentage will. In some cases it may end up being the same thing that they would use anyway.
Because one of the problems with leaving this to plugins is standardized theme adoption, I'd like to see a core-blessed feature plugin be developed first, with an appropriate level of documentation and outreach to seek broad support. If enough themes adopt it and users use it, we could consider this for core with a much better understanding of the need for it, and if not it could remain a standard plugin that themes can support. Such a plugin could also make the appropriate adjustments to the site icon UI to explore ways to improve the relationship between the two - something feels off even with the latest mockup. Site icon starts to take up quite a lot of vertical space, and demoting the placement of the more fundamental site identity options in favor of a more visual option makes it seem like site icon might even belong elsewhere; perhaps reorganized in a section that also combines header and background images for visual/front-end imagery. This project does seem a bit rushed for 4.5, though; I'd hate for this to go in and then cause trouble in the future when we can't remove or significantly change it. A feature plugin seems like the most appropriate way to judge whether this is in fact something that should be in core and to refine the UI further. (also somewhat echoing Sam here).
#37
in reply to:
↑ 36
@
9 years ago
Replying to celloexpressions:
This is a cool feature, but I'm still not convinced that this is core material. My primary question remains, for how many users will there be a need for a site logo that is distinct from a site icon (favicon)?
If I had to guess, I would say for many users. Site icons are full of restrictions. They are usually designed in a square canvas and have to look good in small dimensions. Logos are free from these restrictions.
I wouldn’t be surprised if most sites featuring both a logo and a favicon had a different design for each of them.
I think for many users a Logo feature would be even more useful than the Site Icon. I’m not completely sure about it, but I think many, if not most, people would consider a logo on their page more important than a favicon.
The questions you make could also be made swapping the Logo and the Site Icon features (i.e., if the core already had a Logo feature and we were discussing the inclusion of Site Icon). That’s why I think it makes sense to have both of them (or even none of them). Both are equally important for me. I’d even give the edge to the logo, but that’s just me...
#38
@
9 years ago
We need to answer some questions before jumping into implementing anything like Jetpack.
- What problem is this solving?
- Who is this for?
- Does this solution need to be global? (stays set no matter which theme is active regardless of theme support)
My opinion:
This should be a theme-specific setting. The theme has a better idea of what size restrictions there are and can possibly help with positioning. Why not let the customizer do it's thing? Can't a theme add a site logo image form of some sort?
More importantly, treating it as a standard image addition tool for themes means the theme can add other image/logo areas that aren't separate from the global one in the UI. I've used themes that require a full logo and a small logo (for collapsed fix nav). It would be really frustrating if I could set the full one in one spot and had to hunt for the other.
If it becomes a global, I think it will ultimately cause more confusion and struggle for the user.
#39
follow-ups:
↓ 40
↓ 51
@
9 years ago
This should be a theme-specific setting. The theme has a better idea of what size restrictions there are and can possibly help with positioning. Why not let the customizer do it's thing?
That's exactly what this feature is doing — providing a standardized way for themes to offer logo support, exactly like header and background images. This UI won't just show up randomly, it will only exist if a theme tells it to exist.
The theme goes:
"Hey, I want people using my theme to be able to add a logo. I want it to be positioned here and have this kind of sizing."
WordPress goes:
"Cool! I'll add a UI for you in the Customizer."
Boom, logo.
Can't a theme add a site logo image form of some sort?
No, not yet — not without writing something custom. That's why there are dozens of ad-hoc logo implementations, which is awful for standardization across themes. If you change your theme, you lose your logo. If we add a feature that creates logo support, you don't lose your logo if you change to another theme that supports logo.
What problem is this solving?
There are dozens of ad-hoc ways for themes to add logo support. It lacks standardization and makes it confusing for users. It sucks for theme developers, because they have to roll their own. It sucks for theme compatibility.
Who is this for?
Theme developers and businesses large and small, nonprofits, magazine, anyone who needs a logo or is already using a logo and might one day also change their theme or migrate their content to a new site. This is a huge number of users.
Does this solution need to be global? (stays set no matter which theme is active regardless of theme support)
No, that's not what this is doing. This was never going to be global. This is not a global setting, it's an add_theme_support()
.
I'm honestly shocked this has been so controversial. In all seriousness, have y'all used premade themes, rather than rolling your own? There is a distinct lack of standardization for this kind of feature and it makes live harder for everyone. This is a great step forward to making the lives of both theme developers and theme users easier.
As to making it a feature plugin, it has been tested in a plugin — Jetpack. It's been a Jetpack feature since 2014. Maybe we could ask Jetpack support what their experience with this feature has been and how often people have questions about using it?Someone could also try to see how many themes are already using it by searching for the add_theme_support
call in the .org theme directory.
Also, the biggest argument seems to be "it'll conflict with Site Icon and that's confusing" — but I don't think that's the fault of Logos, but of Icons, which are confusing, have a confusing UI in the Customizer, and they forego industry standard terminology (favicons) so people can't even Google to figure out what the feature is. Rather than derailing Logos, maybe we should be thinking about making Site Icons more distinct.
I wouldn’t be surprised if most sites featuring both a logo and a favicon had a different design for each of them.
I agree with this. Most logos also make terrible favicons, since they usually contain text. A favicon should just be a logo mark.
#40
in reply to:
↑ 39
@
9 years ago
Replying to melchoyce:
Maybe we could ask Jetpack support what their experience with this feature has been and how often people have questions about using it?
We get very few questions about this feature. I'd argue that most site owners don't even know the feature is part of Jetpack. Much like header images and background images, the option appears in the customizer when the theme supports it, so the feature gets associated with the theme rather than Jetpack.
It's also fairly straightforward to use, so doesn't generate that many questions.
#41
@
9 years ago
Hi there - since mid-2014 when the Jetpack Site Logo was first introduced, I've supported several dozen themes that include the feature, including very popular ones like Sela, Edin, and Illustratr.
Users love the Site Logo as it's simple to use and stays intact if they switch to another theme supporting the feature.
I can't recall a single case where a user was confused about how to use the Site Logo, nor a single instance where a user confused it with the Site Icon.
The most popular question I get around logos is (wait for it) how to Make The Logo Bigger, which I guide them through, using a custom function in a child theme and sometimes some complementary CSS.
The occasional user wants to implement a Site Logo without having to rely on Jetpack; these folks will be thrilled to have it in Core.
#42
@
9 years ago
I like the site logo concept for core, especially as it makes logo management more transferrable between themes.
I also think Mel's design for site icon context is important, but especially if the logo is going into core. Two images without context will be even more confusing that site icon currently is. But with the app/favicon context on site icon, the whole thing will be much more clear.
#43
in reply to:
↑ 34
;
follow-up:
↓ 45
@
9 years ago
Replying to melchoyce:
I think we should bring the preview UI from inside the Media Modal into the panel itself.
I like it!
#44
@
9 years ago
I believe this site logo feature in core will enormously welcomed from our users. Jetpack site logo was a big hit in WP.com and people love it when they see the logo stays between themes.
Maybe adding a little note mentioning the max size that theme specifies would help users. Similar to what we do in Header Image.
I really like @melchoyce's design that has context for the site icon. It's clear and reduces the potential confusions.
#45
in reply to:
↑ 43
@
9 years ago
Replying to melchoyce:
I think we should bring the preview UI from inside the Media Modal into the panel itself.
@melchoyce Love this idea; your mockup looks great, I also like the way the logo is at the top... Everything is so much clearer.
This ticket was mentioned in Slack in #core by mike. View the logs.
9 years ago
#47
follow-up:
↓ 48
@
9 years ago
- Version set to trunk
33755.3.diff adds the new Site Icon display as suggested by @melchoyce in her comment and function docs for template tags.
#48
in reply to:
↑ 47
;
follow-up:
↓ 49
@
9 years ago
Replying to obenland:
33755.3.diff adds the new Site Icon display as suggested by @melchoyce in her comment and function docs for template tags.
Great! Thanks for working on this. I had started as well, but you got further anyway. Anything remaining here that needs work other than doing more testing?
#49
in reply to:
↑ 48
@
9 years ago
Replying to adamsilverstein:
Anything remaining here that needs work other than doing more testing?
Yes, we should find a better way to make the "Display Site Title and Tagline" option available for themes that don't support custom headers but do support logos. The current way is rather hacky.
I'm also inclined to say that WP_Site_Logo:: header_text_classes()
should probably be its own function and hooked to wp_head
through a default action. I just wasn't sure where to best put it.
#50
@
9 years ago
I haven't yet tested the patch, but from the diff, I understand that the control is added only when the theme supports site-logo.
If #35807 can be done for 4.5 (use the site logo in wp-login.php), I think there's no need to remove the control in the customizer when the theme doesn't support site-logo.
#51
in reply to:
↑ 39
@
9 years ago
Replying to melchoyce:
This should be a theme-specific setting. The theme has a better idea of what size restrictions there are and can possibly help with positioning. Why not let the customizer do it's thing?
That's exactly what this feature is doing — providing a standardized way for themes to offer logo support, exactly like header and background images. This UI won't just show up randomly, it will only exist if a theme tells it to exist.
Does this solution need to be global? (stays set no matter which theme is active regardless of theme support)
No, that's not what this is doing. This was never going to be global. This is not a global setting, it's an
add_theme_support()
.
If we're talking theme-specific, why is it an option
, and not a theme_mod
? I think that distinction could be confusing, but it also could make sense if properly applied. If it's a theme mod, the theme can specify suggested sizes, etc. If it's an option
, it's global, even if the UI isn't always shown.
It is really not difficult for themes to implement a site logo, or even to do it in a way that will play nicely with other themes if the option
route is desired:
add_action( 'customize_register', 'core_33733_customize_register' ); function core_33755_customize_register( $wp_customize ) { $wp_customize->add_setting( 'site_logo', array( 'type' => 'option' ) ); $wp_customize->add_control( new WP_Customize_Image_Control( $wp_customize, 'site_logo', array( 'label' => __( 'Site Logo' ), 'section' => 'title_tagline', ))); } // in header.php, etc. <img class="site-logo" src="<?php echo esc_url( get_option( 'site_logo' ) ); ?> ">
Also, my primary question remains to be answered. I have no doubt that people like the feature in Jetpack, or that it can be made to work alongside site icon (though I do still think there is redundancy in a lot of cases), but for how many users will there be a need for a site logo that is distinct from a site icon? This really needs to be quantified in some way before we merge this into core; not sure what percentage is appropriate but this wouldn't necessarily follow the 80/20 rule because of theme support. We're making too many assumptions right now.
Aside: I like the redesign of the site icon UI, but that really needs to go into its own ticket. Tying it in with site icon feels wrong, regardless of when/how it comes into core. There is a substantial enough amount of code required for the changes that this piece needs to be broken out and reviewed separately for UI and code.
#52
follow-up:
↓ 53
@
9 years ago
@celloexpressions, it is not only a useless feature, its implementation will actually hurt more then help.
Modern sites have minimum two logos, a desktop and a mobile. Now for a mobile you will want a retina version as well as a "plain" one. In addition the direction in which modern site design goes, you have two distinct logos for "above the fold" area and for the main menu (take a look at techcrunch), therefor there is no point talking about A logo, you need to be able to set several logos and when they should be used, otherwise themes will still need to add their own options for the additional logos which will just confuse users.
Before smartphone became popular I would have agreed with the feature, but now it is just pointless.
#53
in reply to:
↑ 52
@
9 years ago
Replying to mark-k:
@celloexpressions, it is not only a useless feature, its implementation will actually hurt more then help.
Modern sites have minimum two logos, a desktop and a mobile. Now for a mobile you will want a retina version as well as a "plain" one. In addition the direction in which modern site design goes, you have two distinct logos for "above the fold" area and for the main menu (take a look at techcrunch), therefor there is no point talking about A logo, you need to be able to set several logos and when they should be used, otherwise themes will still need to add their own options for the additional logos which will just confuse users.
Before smartphone became popular I would have agreed with the feature, but now it is just pointless.
Your opinion, which you've already made pretty clear under your other username, isn't really helping this discussion. If you could provide something other than an anecdotal reference to how the logo for a huge, custom tech news site works, I think your points might be useful for improving the feature once it's been implemented. Failing that, it seems like you're just working to block something that you don't personally like for whatever reason.
It's already been stated by people from .com and the JetPack team that users like the feature when it's available and that it generates minimal support overhead. That seems like strong enough evidence to justify including it to me.
The entire point of this feature is to standardize something that's already being done in the wild in a myriad of different ways. If WordPress core can't provide a standard for something that's already in-use and then iterate on it to make it better, I'm not even sure what it is that we're doing here.
#54
@
9 years ago
@fatmedia paranoid much?
to spell it out the most used themes in the community are the default themes and neither of them have the "site logo" option. Please contribute code to 2016 since it is its aim to be the technology show case and then people will be able to make a real life assessment of how this feature will be used by real life users and integrators.
As for the reasoning that "some popular plugin does A therefor it should be in core", if that was how core was developed then we would have had SEO, sitemaps and facebook integration by now (all of them also exist in jetpack AFAIK)
#55
@
9 years ago
I like this addition. I've had to add my own logo setting to the customizer before, it would be welcome to have in core, and distinct from the (fav)icon setting. And of course establishing a standard would be helpful in reducing the need for ad-hoc implementations such as mine.
I also agree that the presentation of the (fav)icon section needs a little work to make clearer what it is for, Mel's concept is a good step in that direction.
#57
@
9 years ago
I think I may have stumbled on the same thing @ryan reported. I have been trying to get the patch working so I can do user tests. However, I have ended up with the following lovely image:
I have tried svn clearing, removing all files and then reapplying the patch numerous times - same result each time. I'm using the latest patch and the branch of TwentySixteen.
#59
@
9 years ago
I'm not able to reproduce the console.log()
error (although it makes sense that we wouldn't want to have console.log()
in the patch), but am able to reproduce the error after crop in: Screen Shot 2016-02-18 at 3.45.40 PM.png
Top line: Uncaught TypeError: Cannot read property 'url' of undefined
#60
@
9 years ago
My bad, that should obviously not have been in there. .4.diff should take care of that.
#61
@
9 years ago
33755.5.diff moves the logo selection to the top, above title, as suggested by @melchoyce's mockup. May need a little spacing adjustment, I like the way logo and icon get clearly separated with this layout
Looks like this:
#62
@
9 years ago
I'm still seeing broken behavior when cropping with a site icon ("change image" -> select or upload -> select area -> crop) after 33755.5.diff.
Results in the same error: Uncaught TypeError: Cannot read property 'url' of undefined
Haven't been able to track down the cause yet.
#63
@
9 years ago
Just looping back, it now works for me but I get the site icon hanging still like @mikeschroder reports.
This ticket was mentioned in Slack in #core by chriscct7. View the logs.
9 years ago
This ticket was mentioned in Slack in #themereview by omo. View the logs.
9 years ago
#67
@
9 years ago
33755.6.diff corrects the cropping issue
#68
follow-up:
↓ 70
@
9 years ago
Thanks @adamsilverstein I'll start user testing tmrw with the latest patch.
#69
@
9 years ago
33755.7.diff removes the extra spacing below the logo selection so its more evenly spaced with other controls in this section.
#70
in reply to:
↑ 68
@
9 years ago
Replying to karmatosed:
Thanks @adamsilverstein I'll start user testing tmrw with the latest patch.
Great, thanks!
#71
@
9 years ago
33755.9.diff removes the fixed 100% width for the logo image; works better with smaller images (they were getting resized to fit the column).
#72
@
9 years ago
I'm not saying this is necessarily required for Site Logo, since the rendering logic in the_site_logo()
is not very complicated, but I did want to mention that Selective Refresh (#27355) could be used here to preview changes to the Site Logo.
Here is the code that could be added after adding the site_logo
setting and control:
<?php if ( isset( $this->selective_refresh ) ) { $this->selective_refresh->add_partial( 'site_logo', array( 'settings' => array( 'site_logo' ), 'selector' => '.site-logo-link', 'render_callback' => function() { return get_the_site_logo(); // (Naturally add compat for PHP 5.2.) }, 'container_inclusive' => true, ) ); }
By using Selective Refresh, the preview would be guaranteed to display any changes that maybe getting applied to the Site Logo via the get_the_site_logo
filter.
Something that this also provides is the ability to hover over the site logo to see a tooltip “Shift-click to edit this element”, which when clicking expands the Site Identity panel in the preview and focuses the image control. (See also #27403 for specifics on this UI.)
I've implemented these changes in 33755.8.diff. It also includes the “shift-click to edit” implementation even if selective refresh is not available.
With these specific changes, the selective refresh will apply immediately after the JS-driven update. The duplicated JS preview logic could be removed and the site logo changes could continue to be seen. Nevertheless, having the preview logic implemented in both the JS and PHP would allow the JS update to serve a low-fidelity preview immediately, with a high-quality preview (with PHP filters applied) coming from the PHP-rendered selective refresh after the Ajax request returns.
#73
@
9 years ago
33755.10.diff rollup patch
includes 33755.8.diff and 33755.9.diff
#74
@
9 years ago
@westonruter and @adamsilverstein I was about to user test using the latest patch but if you have no logos selected you can't click to add a logo. It doesn't pop up to add one. This was using 33755.12.diff.
If that can be resolved as would be great to get the user testing running. It may be that this new bug only occurs on installs that haven't had a logo added. I can't replicate on ones that have.
This is what I see on new install that hasn't had a logo:
On an install that has had a logo I see this:
Both are using Twenty Sixteen and both using same patch.
This ticket was mentioned in Slack in #core by drebbits. View the logs.
9 years ago
#76
@
9 years ago
Closing Slack loop on the issue I reported. @adamsilverstein debugged on our testing server and it looks like something in trunk potentially. It's now working and going to head into user testing. Thanks for all your hard work @adamsilverstein.
#77
@
9 years ago
I have run two user tests which are now complete and offer some interesting feedback on this feature: see tests here
A few points to make from this:
- The difference still may be as clear as to what site icon and logo actually are.
- Visually that section is blurring a little, perhaps we can add lines and spacing?
- Discoverability of the feature isn't great. Once someone knows where it is then this solves, perhaps this is ok as a theme feature. Perhaps we need to ensure publicity of this feature and documentation.
- Once people find it and use it the actual process makes sense.
- The cropping force on site icon perhaps could be removed. This caught a user. Maybe have same format as logo?
I am happy to run more tests, but also feel we have some good feedback here to digest. Let me know if you want more tests run.
#78
follow-ups:
↓ 79
↓ 80
@
9 years ago
This feels incredibly rushed to me.
The first patch appeared 10 days ago, and 4.5 beta 1 is next week. I'm skeptical that we can ship something polished and user-test-iterated by then without pulling a lot of resources from other areas also rushing to hit the beta 1 deadline.
Citing the initial user tests @karmatosed documented in comment:77, I think inclusion of a Logo feature should run through the full feature plugin process before it ever gets to a point of core inclusion.
Let's not skip steps in the name of enthusiasm for shiny things, I think the implementation will ultimately suffer.
#79
in reply to:
↑ 78
@
9 years ago
Replying to DrewAPicture:
This feels incredibly rushed to me.
The first patch appeared 10 days ago, and 4.5 beta 1 is next week. I'm skeptical that we can ship something polished and user-test-iterated by then without pulling a lot of resources from other areas also rushing to hit the beta 1 deadline.
Citing the initial user tests @karmatosed documented in comment:77, I think inclusion of a Logo feature should run through the full feature plugin process before it ever gets to a point of core inclusion.
Let's not skip steps in the name of enthusiasm for shiny things, I think the implementation will ultimately suffer.
+1.
Enough people have voiced concerns over both the need for this in core and the rush to get it into 4.5 that it's worth taking a step back. Jetpack isn't a core feature plugin. Let's make one with a version of the feature as we would see it in core, per comment:36 (last paragraph).
Per my previous comment (comment:51), improvements to the site icon feature should be considered separately. That part could potentially make it into 4.5 if a separate ticket is created. If we're only making those changes because of confusion stemming from site logo, we need to reconsider site logo. Also, moving site logo above title and tagline because the separation clarifies the difference between them seems like a bad approach. We can look at these issues and general placement during the feature plugin process.
#80
in reply to:
↑ 78
@
9 years ago
Replying to DrewAPicture:
This feels incredibly rushed to me.
The first patch appeared 10 days ago, and 4.5 beta 1 is next week. I'm skeptical that we can ship something polished and user-test-iterated by then without pulling a lot of resources from other areas also rushing to hit the beta 1 deadline.
I agree that the development of the feature has been quick, but most of the contributors working on it have been separate from other areas, and/or those areas have already hit their deadline, so I don't feel like it's going to hurt other projects.
Citing the initial user tests @karmatosed documented in comment:77, I think inclusion of a Logo feature should run through the full feature plugin process before it ever gets to a point of core inclusion.
I'm pretty mixed here. The user tests were interesting. One bit of feedback that was good (and makes me lean in the direction you've mentioned) from the first test is that the user initially looked in "Header."
That said, most of the difficulty the users faced seemed to stem from them not recognizing the Customizer as the place to go to modify the way their site looked, rather than a problem with the logo feature itself. This makes me wonder if there's a way to test in a way that does not reflect these problems -- which we should definitely tackle -- on a feature that we're separately considering.
#81
@
9 years ago
Site Icon was introduced later and in a way worse state than Logos are now in. It didn't even have Customizer support at that point. Logos are way more polished, and like Icons have been tested in a plugin for a long time. Features don't need to have existed as a plugin first before they are allowed to be added to core, and I don't think we should start making that a requirement. I think it's fine to use beta to refine the feature if necessary, we've done that successfully in previous releases as well.
This ticket was mentioned in Slack in #design by mike. View the logs.
9 years ago
#83
@
9 years ago
I'll just add a note of caution in seeing those user tests as totally negative. There are always a ton of sides to any user test.
This feature was super hard to show unless gave exact instructions on where to find it, because of the nature of the Customizer. The majority of user test that involves allowing someone to find something in or the Customizer itself will have the Customizer hunting going on. That's why I added steps for those truly lost. We had to have that discovery just to see if people had an idea.
Now there are some important things that we could act on right now to help beyond the seeking Customizer. My suggestions for hopefully easy fixes are:
- Adding separators like: https://core.trac.wordpress.org/ticket/35040.
- Make both site logo and site icon have the same crop or just use behaviour.
I feel fixing discoverability of the Customizer is beyond this feature. But, take it from these tests as really being an area needs consideration.
#84
follow-up:
↓ 88
@
9 years ago
There seem to be a few objections here. Let me make sure I'm understanding them.
- "Site logo" doesn't belong in core.
- "Site logo" is too confusing when compared to "Site icon."
- "Site logo" has not received enough testing (including user testing) yet.
- "Site logo" should be developed and iterated in a feature plugin before inclusion.
- The code for "Site logo" is new and might not be high quality.
I don't really agree with any of these objections, for these reasons:
- There's clearly a need/use for site logo as evidenced by the usage it receives in Jetpack. Feature use in Jetpack isn't always an indication that a feature belongs in core, but given the (positive) reception of the site icon feature in 4.3 and similar positive indicators from users who want/use site logo, I think in this case it's well-warranted for core.
- This is the biggest issue I had initially, but user testing didn't show this to be a real issue. We could do some work here to remove any remaining confusion (see comment 83), but that shouldn't stop site logo from going into core.
- So far, there have been (only) a few user tests of site logo, but it has also received general user testing in the form of shipping in Jetpack. I can't find the comment (perhaps it was in Slack, perhaps I just can't find it amid all these other comments), but the Jetpack team noted that this feature received very few support requests except as it related to the naming, an issue we're improving by calling it "Logo" instead of "Site logo" in the UI. I don't think we've ever had a set minimum for users tests of a feature that goes into core (how many did the Customizer theme switcher have? how about the "new" Press this?), but two to three are usually acceptable, depending on the results.
- Features do not have to be developed in plugins. We have never mandated this. While it's certainly preferred in some cases, it's not necessary in others. This "requirement" should be case-specific. In this specific case, the site logo UI is very similar to site icon (a successful core feature) and to Jetpack's site logo feature. No, Jetpack is not a "feature plugin for core" but it is a great place to watch features progress and decide if they should migrate to core. There is nothing wrong with pulling features from Jetpack as we're doing here. Further, features in Jetpack see higher usage than any feature plugin we've ever had.
- I don't actually see anyone making this explicit argument, but I wanted to address it. With core committers developing this feature and reviewing code quality, this really shouldn't be an issue. The speed of which something is developed does not indicate its quality.
If there are other objections, I clearly missed them. I think my point here is that if site logo is testing well, has a core-approved UI that we like, there's no reason to hold it back. I haven't seen evidence to the contrary.
#85
@
9 years ago
My confusion around the introduction of the site logo feature came almost entirely from not knowing this was a theme modification. I had 2 questions:
- Don't we already have site icons?
- Didn't we already have Blavatars? (We don't, FYI)
I think "Logo" is the appropriate terminology, but I wasn't surprised that this feature went from suggestion to core inclusion quickly, for the following reasons:
- It's already in a vetted plugin with active installations
- It's not really very much different than the other 3 places where the Customizer attaches images to places (header, background, icon)
- It's a very logical progression from other theme-side enhancements, that a majority of WordPress powered sites have some semblance of a logo
With these ideas in mind, I'm not sure the number of days matters very much, and I don't consider feature-plugins to be a literal requirement yet (that debate belongs in a different place, and I hope it never becomes one) but it seems like this feature doesn't need very much more massaging to be both good for users and acceptable to us.
The value of feature plugins, from my perspective, is it gives everyone the opportunity to review the idea and result. It's less about build-to-merge, and more about experimenting with approaches in a safe place until everyone feels the idea is viable. And this particular feature was already viable in Jetpack (and various themes have been doing this bespoke for years) and we all seem to agree it's a good idea.
If any feature could feel rushed, it would be this one. It's small, universally accepted, and there's lots of prior art to make it less difficult to build.
#86
@
9 years ago
While I'm not personally convinced (yet) that this feature will be used enough to warrant core inclusion, I don't get the impression that any numerical data or other outreach efforts will be conducted prior to making a decision. For some reason, several people really want to see this happen ASAP. Fine, but we need to make sure we're all on the same page.
My biggest concern is the definition of this feature as a "theme" feature. There are multiple comments over the past week suggesting that this is a theme-specific feature. @michaelarestad and @melchoyce constructed a strong argument for logos as a theme feature in comment:38 and comment:39, @johnjamesjacoby specifically mentioned "theme modification" in comment:85. However, in the patches, the setting is explicitly an option
. This means that the same logo will be used when you use a different theme. It is not theme-specific, it is global. However, the UI is only shown when a theme explicitly adds support via add_theme_support()
. This is somewhat different from what has been discussed on the ticket, but may still be okay with everyone and could be described as a theme feature in many ways.
I'm worried, though, about the suggestions to possibly turn logo support on automatically regardless of theme so that it can be used on the login screen or elsewhere (#35807). As soon as that possibility comes up, we're no longer talking about adding a feature that themes can turn on, we're talking about a new option for all sites, and one that's likely to detract from the proper use of options from site title and tagline to site icon, which are critical to a site's identity well beyond the front-end/the site itself. We need to consider "decisions, not options," and whether this is an option that will be used enough to warrant the potential consequences. Personally, I think this will overlap with site icon for many users. In terms of user testing, I would want to see a couple of tests that attempt to determine whether this reduces proper usage of site icon and the title and tagline fields; usertesting.com tests probably aren't the best way to gauge this. And the logo should definitely be the last control in the site identity section - title, tagline, and icon are ultimately more important to the identity of a site because they affect its representation externally, whereas the logo is identity that is shown only within the site itself. The placement at the bottom also makes the most sense for a feature that is only available with certain themes, otherwise the placement of the other fields moves around.
If nothing else, this feature warrants a make/core post for broader feedback before a final decision is made.
I have a few additional technical comments (specifically relating to the use of a custom customizer control), but I'd like the dust to settle a bit first so that they don't get lost behind 20 comments in a day here. The patch definitely isn't committable just yet, so there's work to be done and I do think that the feature will be better-implemented with more time, even if that means introducing it after the first beta if there's some reason I'm missing that this absolutely has to make it into 4.5. This isn't a code quality issue as much as it's about making sure that the feature is implemented in the best approach for core and taking care of some detail items. It's going to be much more effort to go back and adjust after it's committed than to get it better the first time.
#87
@
9 years ago
For what it's worth, I think there is a reason that the implementation uses an option even though it's theme-specific. Because a company logo is often tied to a lot more than just a website, it tends to be something that will persist from theme to theme, but it's also something that must be accounted for in the design in order for it to look right.
If the setting used a theme mod, it would lose persistence and potentially add another useless step for the user. If it wasn't tied to theme support, it would show up in themes where there's been no design work done for the logo and it could (probably would) look really terrible.
#88
in reply to:
↑ 84
@
9 years ago
Replying to samuelsidler:
I don't really agree with any of these objections, for these reasons:
<snip>
- Features do not have to be developed in plugins. We have never mandated this. While it's certainly preferred in some cases, it's not necessary in others. This "requirement" should be case-specific. In this specific case, the site logo UI is very similar to site icon (a successful core feature) and to Jetpack's site logo feature. No, Jetpack is not a "feature plugin for core" but it is a great place to watch features progress and decide if they should migrate to core. There is nothing wrong with pulling features from Jetpack as we're doing here. Further, features in Jetpack see higher usage than any feature plugin we've ever had.
- I don't actually see anyone making this explicit argument, but I wanted to address it. With core committers developing this feature and reviewing code quality, this really shouldn't be an issue. The speed of which something is developed does not indicate its quality.
To be fair, I'm not suggesting this be developed first as a feature plugin just because.
I'm suggesting it be developed as a feature plugin first because the process implies iteration has occurred, with a result devised from a combination of unbiased core consensus, gathered data, and user testing in an open forum. Development and iteration (even by core developers and designers) inside the Jetpack/WP.com bubble doesn't qualify as unbiased consensus, in my opinion.
Please do not misunderstand my motives in suggesting we take more time to iterate. I think this feature will be a huge hit with the user base. However, I'd like feel like we got it right the first time.
#89
@
9 years ago
Other points:
- I really like @melchoyce's suggestion for improving the site icon UI as part of reducing confusion. It would be really interesting to see the effect that might have on user test results in conjunction with a logo setting.
- I agree that a single set of user tests alone cannot be considered conclusive. However, barring any other data to consider, I remain skeptical that we're putting our best collective foot forward in rushing a merge
- Furthermore, I agree with @celloexpressions's position that we need to decide whether this is a theme-specific or global feature. I have zero problems with implementing this on a global scale and simply making it straightforward for themes to leverage it, but we have to pick one.
#90
follow-ups:
↓ 91
↓ 93
@
9 years ago
I think that items localed in the Site Identity are global things that can be used both on front-end and back-end.
What do you think of the idea to use the site logo in wp-login.php (#35807)?
#91
in reply to:
↑ 90
@
9 years ago
Replying to benoitchantre:
I think that items localed in the Site Identity are global things that can be used both on front-end and back-end.
What do you think of the idea to use the site logo in wp-login.php (#35807)?
The important thing about images is to keep their aspect ratio. As long as you keep it and downsize or upsize by a reasonable factor nothing horrible will happen, but downsize too much and you just get a mesh of colors and upsize too much and you get pixelization. The problem with the site logo as a global resource is that there are no standards for the logo size and if theme A have only 100 pixels available while theme B has 200 you will either end up with pixelization if you upsize or some ugly empty space if you don't. Switching from a logo optimized for B to A will result in a logo which is not displayed to the optimum especially if it contains text.
Using the login logo as the site logo makes IMO more sense as everybody knows what is the relevant size and can plan for it. Down side is that it will "hardcode" the size of the logo in the login screen, but I don't remember that it had ever changed since the current design was introduced long time ago.
This ticket was mentioned in Slack in #design by helen. View the logs.
9 years ago
#93
in reply to:
↑ 90
;
follow-up:
↓ 95
@
9 years ago
Replying to benoitchantre:
I think that items localed in the Site Identity are global things that can be used both on front-end and back-end.
What do you think of the idea to use the site logo in wp-login.php (#35807)?
My view is that this sounds like something that would be better left in plugin territory.
That said, happy to have discussion on that idea happen in #35807. Let's focus on theme logo support here.
This ticket was mentioned in Slack in #core-flow by boren. View the logs.
9 years ago
#95
in reply to:
↑ 93
@
9 years ago
Replying to mikeschroder:
Replying to benoitchantre:
My view is that this sounds like something that would be better left in plugin territory.
That said, happy to have discussion on that idea happen in #35807. Let's focus on theme logo support here.
Thanks Mike for your feedback. I thought that it would help the discussion about the persistence (or not) of the control in the Customizer.
#96
@
9 years ago
I tested .12.diff on an iPhone and added screenshots and an update to the end of this post. Site icon flow looks better and no longer completes without feedback.
https://make.wordpress.org/flow/2016/02/19/customize-site-logo-and-icon-33755-5-diff-iphone-6/
#97
@
9 years ago
@celloexpressions, I agree that it would be confusing to users if we add this as a global option, and the logo settings disappear when switching to a theme that does not support it, while the logo may continue to be shown in global context elsewhere.
Additionally, since themes can set different sizes for these logos, if we go with a global option, would it be setting up users for a bad experience since the size isn't regenerated when they change themes? This likely isn't a problem for Jetpack due to Photon, but is an issue for core, unless I'm missing something here.
Due to these reasons, currently leaning towards a theme mod for 4.5, which would avoid these issues, but not prevent moving to a global option in the future if desired.
This ticket was mentioned in Slack in #core by jorbin. View the logs.
9 years ago
#99
@
9 years ago
I think we should firmly mark this as "Theme Logo Support", change the storage to be theme-mod, and move forward.
Chatted with @DrewAPicture about this, and he concurred.
Does anyone have the time to update the patch to reflect this before tomorrow?
Agreed that a make/core post is important. @adamsilverstein is prepping one, and if there are consensus issues that arise either here in the ticket or there, we'll discuss and reevaluate.
#100
@
9 years ago
- Keywords needs-refresh added
I can do a sweep on the technical concerns I had tonight and make the other update on the patch to do a theme mod. Would try to position it for a somewhat more conservative approach that can be expanded on during beta for the more complex changes, part of it definitely couldn't happen before tomorrow.
As of right now, there isn't anything that I saw in the patch that would let themes specify a logo size. If it is a theme mod, this would be a good use case for a cropped-image control, but with logos it's probably more important for the content creator to dictate the size/aspect ratio and the theme to need to adapt as needed. I also do think this is only slightly more convenient as a core theme-supported feature versus recommending to add an image control. The selective refresh support makes the core theme feature approach more compelling, though.
#101
@
9 years ago
- Keywords needs-refresh removed
33755.13.diff switches to storing the logo in theme-mod.
@
9 years ago
Remove unnecessary custom Customizer control and add needed functionality to core controls.
#102
follow-up:
↓ 105
@
9 years ago
In 33755.14.diff:
- Remove the custom customizer control in favor of the core-provided
WP_Customize_Media_Control
. A site logo is not a complicated usage of an image control. A logo control is a great example of a use case for the standard image control that ships with core. In fact, the UI in the patches is essentially the same as the core image control. Core customizer controls are a demonstration of how to interact with the customizer API, and this complex usage is unreasonable for a theme or plugin to consider when evaluating the approach to adding their own controls. Themes and plugins should be able to accomplish what the previous patches accomplished with a custom control with the existing core-provided controls, and core should too. We can't launch this feature with the custom control class, then go back and remove it later, because customizer controls added to the API become available for general use - themes and plugins could start using it and core gets stuck with maintaining it. The custom control adds a lot of bloat, most notably in the ever-ballooning list of CSS selectors for all of the media control styles, for very little benefit (also note the significantly reduced patch size, and that even if we were to keep a custom control, the implementation could be simplified and better future-proofed without functional changes). Due to the time crunch, the patch removes the custom control and associated code. I was able to roughly implement the custom things (see items below), but someone should review as I don't have time to fully test and debug tonight. We can't ship with a custom logo control and go back and change it, though, so we'll need to live with slightly different button labels and no postMessage if it can't be figured out soon. - Don't put the logo control at the top of the site identity section. We could claim that it's the most important setting, but being firmly theme-specific, putting it there would cause the other options to move around, which would be disorienting. Additionally, it makes sense to put global options before theme options, since they're probably more important to set. This area could use further improvement, but placement at the top feels like a mistake, much like the former behavior of custom customizer sections defaulting to priority 0 (placement at the top). Let's not let this hinder existing site-wide settings, especially not in a first pass.
- The
button_labels
parameter is supposed to be customizable, but I messed that up when I wrote the originalWP_Customize_Media_Control
and the new image control patches and I guess we never caught it. I added those to the control registration, so that element of the custom control is preserved (may not fully be working yet though - could use another set of eyes). This change is backwards compatible for subclasses that previously set button labels by overriding the whole array in a custom class constructor - that still works. - I added the custom JS that the custom control was doing to the core control - I think it could be generally useful. However, I don't have time to closely test this or look at any potential side effects so someone should review more carefully. If it's not working we should just remove the postMessage support for now and revisit later (it's not worth the custom control). Situations where the setting is not an id could be problematic (instances of
WP_Customize_Image_Control
store a URL).
Additional things I think should be changed or considered:
- the
wp-site-logo
body class seems really excessive. Do we want to get locked in to having this? There are a lot of body classes already, and I don't think there would be common enough usage to warrant it in core; it could be added by a theme if there is a specific need for it. - There is still a
@todo
in the patch. The comment above it doesn't make sense to me, so not sure what's left to do there, but this should be resolved (or clarified) before committing to core. - We could probably consolidate the site icon control display to show the icon next to the browser preview; that would make it take up less vertical space if the logo is below.
- If this is explicitly a theme feature, adding theme support for
site_logo
may not the best terminology. - Can we add support to Twenty Fifteen? I think at least two bundled themes should support this out of the box, possibly more. Fourteen and Fifteen could put it at the top of the sidebar.
Generally, this feels over-engineered to me. However, I don't have time to dig in further, so hopefully cleaning up the customizer parts is enough. This absolutely should not be committed with the custom control though, it dilutes the core objectives of the customizer API and adds a lot of duplicated code that isn't needed. Definitely needs some work tomorrow to finish cleaning up; I won't be around to help but can circle back at the end of the week.
#103
follow-up:
↓ 118
@
9 years ago
- Is
WP_Site_Logo::preview_enqueue()
still needed? It tries to load a script viaplugins_url()
. - Can
_render_site_logo_partial()
be part ofWP_Site_Logo
? is_a()
should beinstanceof
, see [31188].images/browser.png
should be an absolute URL- I think the
restore_current_blog()
call inget_the_site_logo()
is too early, sohome_url()
andwp_get_attachment_image()
will return stuff for the current site. Not sure if that's intended. A unit test might be helpful to check this. - Should "a setting to hide header text" be moved into its own ticket?
#104
@
9 years ago
I added shots of .12.diff on Nexus 5 and iPhone 5 here: https://make.wordpress.org/flow/2016/02/19/customize-site-logo-and-icon-33755-5-diff-iphone-6/
I'll test .14.diff soon.
#105
in reply to:
↑ 102
@
9 years ago
Replying to celloexpressions:
In 33755.14.diff:
- Remove the custom customizer control in favor of the core-provided
WP_Customize_Media_Control
. […]
I don't think I agree here. We already have MediaControl
subclasses for HeaderControl
, BackgroundControl
, and SiteIconControl
(via CroppedImageControl
). Why not also have a class tailored for site logos?
- Don't put the logo control at the top of the site identity section. […]
I don't have a strong opinion about this, but it might be easier to compare/contrast a site logo with the site icon if the two controls are next to each other.
- The
button_labels
parameter is supposed to be customizable, but I messed that up when I wrote the originalWP_Customize_Media_Control
and the new image control patches and I guess we never caught it. […]
This is implemented in the patch for #35542.
- I added the custom JS that the custom control was doing to the core control - I think it could be generally useful. […]
Yes, it would seem to be generally useful. I see no reason why this would cause a problem. The alternative is to add a new event handler outside the scope of a specific control:
wp.customize( 'site_logo', function( setting ) { setting.bind( function( value ) { /* send data to the preview */ } ); } )
Two points:
- Use the setting ID as opposed to the control ID in the message being sent to the preview. Normally these are the same, but the setting is more granular and relevant to what is actually being changed:
var setting = this; /*...*/ wp.customize.previewer.send( setting.id + '-attachment-data', this.attributes );
- Remove debug code:
alert( control.id + '-attachment-data' );
(how has this not been seen in user tests?)
Generally, this feels over-engineered to me. However, I don't have time to dig in further, so hopefully cleaning up the customizer parts is enough. This absolutely should not be committed with the custom control though, it dilutes the core objectives of the customizer API and adds a lot of duplicated code that isn't needed. Definitely needs some work tomorrow to finish cleaning up; I won't be around to help but can circle back at the end of the week.
As above, I don't think I agree. We have specific control subclasses for other image controls, so why not one for a site logo?
#106
@
9 years ago
I feel very strongly about the custom control issue.
The background control, and really the image control (extending the media control), are only there for back-compat. The header image control needs to be refactored to extend (or possibly even be) the cropped image control, with the core control getting library support to show existing options. Site icon and header images both have UI that is very specific to the way those controls are interacted with, whereas a logo control UI is the same as any image control. With the revisions in my patch, once it's cleaned up, there is actually no functional difference - there is nothing that would be different about a logo control versus the core media control. Therefore, there would be no need for a logo control at all. One of the primary use cases for the image control/media control with mime_type=image is for logos and other theme-specific images on the site.
Generally, I feel like the point of having control types that can be registered for different instances with different parameters is the ability for the vast majority of developers to use core controls when creating custom options. Core should try to do this as much as possible as well. In this case there is actually no difference between what an image control vs. a logo control would look like, so it should definitely use the core one. Custom controls are an option that can help when the UX needs to be significantly modified, but in this case there's no reason to use one.
#107
@
9 years ago
I added the alert while testing briefly, that wasn't previously there in the other patches. Like I said, patch is rough, will need some tweaking :)
#109
@
9 years ago
After talking with @westonruter, thinking we should, after a discussion during dev chat, land 33755.13.diff as a first run, while still considering and talking through changes in implementation here, for iteration during beta.
Given @helen and @mor10's great comments on the make post, I think it we should continue to user test and prepare another post, likely as part of the field guide, explaining to theme authors our specific intent with the feature.
This ticket was mentioned in Slack in #core by mike. View the logs.
9 years ago
#112
@
9 years ago
- Type changed from feature request to task (blessed)
Moving this to task for 4.5 beta.
Let's spawn out changes/bugs to other tickets as possible.
#113
@
9 years ago
- Keywords has-patch needs-testing removed
Follow-up so far:
- Fixing the disaster that is
WP_Customize_Site_Logo_Control
: #35941 - Considering different logo control placement: #35942
- Refine new site icon control UI: #35943
- Twenty Fifteen: Add Site Logo Support: #35944
- Reconsider
site
terminology for site logos: #35945
There are several additional items above that need follow-up here or on new tickets; I'll let someone else try to parse those.
#115
@
9 years ago
Hey there,
for me it seems as a useless feature as long wordpress does not support svg images (#24251). Lots of sites uses or want to use svg logos, because of a better quality (no compression), small file-size and already retina-ready. Lots of grafic designers also more and more just provide svg logos and icons, because that's (for good reasons) the new way of embedding logos & icons.
So here I would more focus on implementing svg-support for both logo and favicon (favicon can be rendered to png, if needed on some devices), instead of adding such feature which will not be used by many sites.
Another point, some themes provide options to use default and small logos (e.g. small logos for sticky headers...). I think here with a filter it's possible to simply add another field under the logo field in customizer?
BTW: I like the idea of customizing general theme options in customizer!
#116
@
9 years ago
Is the logo feature going to include any utility functions for retrieving the logo?
e.g. get_site_logo();
If so it would be useful to have a filter for the logo link URL so a theme could easily set this to none, the homepage, or a custom link.
#118
in reply to:
↑ 103
@
9 years ago
- Keywords needs-patch added
Replying to ocean90:
- Is
WP_Site_Logo::preview_enqueue()
still needed? It tries to load a script viaplugins_url()
.- Can
_render_site_logo_partial()
be part ofWP_Site_Logo
?images/browser.png
should be an absolute URL- I think the
restore_current_blog()
call inget_the_site_logo()
is too early, sohome_url()
andwp_get_attachment_image()
will return stuff for the current site. Not sure if that's intended. A unit test might be helpful to check this.- Should "a setting to hide header text" be moved into its own ticket?
I think all of these things still need follow-up, either here or on their own tickets. Also, there are still no unit tests for logos. There seems to be very little momentum now that the first pass is in, but there are still several rough edges that need to be addressed.
This ticket was mentioned in Slack in #core-flow by boren. View the logs.
9 years ago
#120
follow-ups:
↓ 124
↓ 128
@
9 years ago
With this in the nightlies, how about adding site logo support to a bundled theme so we can test out of the box?
#121
@
9 years ago
Using https://github.com/wordpress/twentysixteen/tree/site-logo to test, I see this on one site.
https://cloudup.com/c7hLLMBQn3M
The console shows:
https://cloudup.com/cMMUU_oGS8H
This happens on one site running 4.5-beta1-36797. I don't see anything in the php error logs. A localhost VVV site running 36801 does not show the problem.
This ticket was mentioned in Slack in #core by boren. View the logs.
9 years ago
#123
@
9 years ago
@ryan I just faced that same issue yesterday. I got around it by manually short-circuiting the site logo feature in Jetpack via a little mu-plugin like:
<?php add_action( 'after_setup_theme', function() { remove_action( 'init', 'site_logo_init' ); } );
Either that or deactivating the Jetpack plugin should do it.
#124
in reply to:
↑ 120
;
follow-up:
↓ 125
@
9 years ago
Replying to ryan:
With this in the nightlies, how about adding site logo support to a bundled theme so we can test out of the box?
Twenty Sixteen has support for the site logo and is bundled.
#125
in reply to:
↑ 124
;
follow-up:
↓ 127
@
9 years ago
Replying to ocean90:
Replying to ryan:
With this in the nightlies, how about adding site logo support to a bundled theme so we can test out of the box?
Twenty Sixteen has support for the site logo and is bundled.
I'm not seeing it. https://cloudup.com/cspxCaK55hq
#126
@
9 years ago
Disabling jetpack fixes the problem.
Currently, I don't see site logo with Twenty Sixteen. I must use the site logo branch of the theme to see the logo. Between that and having to disable a major plugin, this isn't testable out of the box with beta 2 nigh.
#127
in reply to:
↑ 125
@
9 years ago
Replying to ryan:
Replying to ocean90:
Replying to ryan:
With this in the nightlies, how about adding site logo support to a bundled theme so we can test out of the box?
Twenty Sixteen has support for the site logo and is bundled.
I'm not seeing it. https://cloudup.com/cspxCaK55hq
Ah, twenty sixteen in the theme directory isn't up-to-date. @karmatosed is looking.
#131
follow-up:
↓ 134
@
9 years ago
@celloexpressions I see you say:
"the wp-site-logo body class seems really excessive. Do we want to get locked in to having this? There are a lot of body classes already, and I don't think there would be common enough usage to warrant it in core; it could be added by a theme if there is a specific need for it."
Actually for themers this is incredibly useful. I would strongly advise we revert this removal. It helps so much with positioning and stops us having to silly hacks and selector statements that are incredibly delicate and easily broken.
#132
@
9 years ago
Defect: #36096 (Use of custom image size requires unexpected filtering of image_size_names_choose)
#134
in reply to:
↑ 131
@
9 years ago
Replying to karmatosed:
@celloexpressions I see you say:
"the wp-site-logo body class seems really excessive. Do we want to get locked in to having this? There are a lot of body classes already, and I don't think there would be common enough usage to warrant it in core; it could be added by a theme if there is a specific need for it."
Actually for themers this is incredibly useful. I would strongly advise we revert this removal. It helps so much with positioning and stops us having to silly hacks and selector statements that are incredibly delicate and easily broken.
I agree it can be useful, but I'm not sure it's important for core to add it by default. If a theme does need it for one reason or another, it's pretty quick to filter it in there.
Another reason it probably makes sense to leave it out of core is that IMO a conditional class would be more useful when applied to the parent container around the logo. The rest of the site likely won't care whether a logo is present, but the site's main <header>
element might, just an example.
#139
follow-up:
↓ 140
@
9 years ago
Codex page created for this theme feature: http://codex.wordpress.org/Theme_Logo
#140
in reply to:
↑ 139
@
9 years ago
Replying to ramiy:
Codex page created for this theme feature: http://codex.wordpress.org/Theme_Logo
Should the Codex page also demonstrate how to add the logo to a theme?
#144
@
9 years ago
- Description modified (diff)
- Summary changed from Add Site Logo to WordPress Core to Add Custom Logo to WordPress Core
#150
@
9 years ago
Ignore. I've split this into two tickets: #36255 and #36256
When an image is uploaded, the expectation is for a cropping UI to show up if that image does not have the same proportions as what the logo area is defined as. Currently (4.5-beta3-36995), that is not the case, and from what I can tell, the only control of crop factor is at the theme level through the add_image_size() function. From a user experience perspective this is sub-optimal.
I would recommend adding cropping functionality similar to what we have for the Site Icon and Header Image functions to the Custom Logo function.
Additionally, the preview in the Customizer sidebar does not reflect the crop factor (see image below: Displayed image in theme is cropped to a square 96x96).
-1
Placing the logo requires CSS changes, and it is actually faster and more future proof to have the logo image as part of the CSs.
So if you want to properly implement such a feature you need to eliminate the need to do any CSS changes and let the user drag and drop the logo to its proper location to get it proper location. Then of course the question is how do you handle cases like positioning the logo in the middle of the width of the site, something that is actually much easier to do with CSS then manually.
And then you also need to supprot a different logo for mobile.
Not really against it, just thinking that value for money is very low in this case. Jetpack has it for feature parity with wordpress.com but in wordpress.com most people can not edit their CSS which is not true for stand alone wordpress.