WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#35944 closed task (blessed) (fixed)

Twenty Fifteen: add support for site logos

Reported by: celloexpressions Owned by: karmatosed
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.6
Component: Bundled Theme Keywords: has-patch needs-testing
Focuses: ui Cc:

Description

I'd suggest putting it above the site title and tagline in the sidebar, and possibly not showing it at all on mobile where the sidebar becomes a header. I've had users specifically want to add a logo to this theme, in this location before. And, we should ship with more than one bundled theme supporting the featured. It could work for twenty fourteen as well, but it's less convincing there.

I'm attaching the rough version I did while testing the feature, it could use the addition of a bottom margin, decision of a proper size setting, and consideration of the ability to hide the site title and tagline when there's a logo.

Attachments (4)

twentyfifteen-site-logo.diff (1.1 KB) - added by celloexpressions 6 years ago.
The raw basics - needs some refinement.
twentyfifteen-site-logo.2.diff (598 bytes) - added by drebbits.web 6 years ago.
35944.3.diff (3.5 KB) - added by iamtakashi 6 years ago.
35944.4.diff (3.5 KB) - added by karmatosed 6 years ago.
Changes site-logo to custom-logo as per latest updates on feature

Download all attachments as: .zip

Change History (22)

@celloexpressions
6 years ago

The raw basics - needs some refinement.

#1 in reply to: ↑ description @drebbits.web
6 years ago

Replying to celloexpressions:

...consideration of the ability to hide the site title and tagline when there's a logo.

They could just use the Display Site Title and Tagline option to pro-actively hide the site title and tagline.

I've provided little edits to the initial diff. I'm thinking placing a reference link in the doc block but that could be updated when we update the codex with site-logo theme support.

#2 @johnbillion
6 years ago

  • Component changed from Customize to Bundled Theme
  • Type changed from defect (bug) to task (blessed)

#3 @iamtakashi
6 years ago

The logo implementation touches the design of the theme so I'm gonna work on this today.

@iamtakashi
6 years ago

#4 @iamtakashi
6 years ago

  • Keywords needs-testing added; needs-refresh removed

I've added the support with some style refinements. The logo is smaller in small screens but but I'm not going to hide it.

The patch assumes the theme to be next version, 1.5.

Last edited 6 years ago by iamtakashi (previous) (diff)

This ticket was mentioned in Slack in #core by melchoyce. View the logs.


6 years ago

#7 @mikeschroder
6 years ago

  • Owner set to karmatosed
  • Status changed from new to assigned

@karmatosed
6 years ago

Changes site-logo to custom-logo as per latest updates on feature

#8 @karmatosed
6 years ago

I've refreshed that patch as the naming has changed to custom-logo. If I could get a second pair of eyes on it before I commit that would be great just as a test. @iamtakashi if you have time would like you to test for me.

The name change for the body is dependent on: https://core.trac.wordpress.org/ticket/35945. Once that's been approved we can then commit this but this patch does depend on this.

If that happens today gmt I'll commit this, otherwise it will be Tuesday later when I'll next get a commit chance as travelling. I'm totally happy someone committing this in my absence though as it may need to just ship :) @mikeschroder I'll let you decide if that patch in dependent ticket drops while I'm away.

Last edited 6 years ago by karmatosed (previous) (diff)

#9 @iamtakashi
6 years ago

Assuming the body class is going to be added back, 35944.4.diff looks good to me.

#10 follow-up: @celloexpressions
6 years ago

Can we clarify in the inline comment that twentyfifteen_the_custom_logo() exists only for backwards compatibility with WordPress <4.5? At first glance it looks unnecessary, and if its purpose is clarified, it could be safely removed by developers basing themes off of Twenty Fifteen if they don't need to support older versions.

#11 in reply to: ↑ 10 @iamtakashi
6 years ago

Replying to celloexpressions:

The check inside it is obviously for backwards compatibility as well as for the case like the theme support is removed in child themes for whatever the reasons, at the same time, it allows the template cleaner.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


6 years ago

#14 @karmatosed
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 36913:

Twenty Fifteen: add support for site logos
Fixes #35944
Props @iamtakashi, @celloexpressions, @drebbits.web

#16 @DrewAPicture
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
Last edited 5 years ago by DrewAPicture (previous) (diff)

#17 @DrewAPicture
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Wrong ticket.

This ticket was mentioned in Slack in #core by obenland. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.