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

Add Site logo as default to the login page #424

Open
wants to merge 15 commits into
base: develop
from

Conversation

@bahiirwa
Copy link
Contributor

commented Jun 19, 2019

Grab the Site Logo, description and title and set that as the logo on the login screen. Since the login logo is wrapped in an <a>. We add the title from bloginfo('description') and href from bloginfo('url')

Description

  • Get the theme logo through customizer.
  • Add logo through core function hooks actions and filters as style with background image if theme logo exists or default to CP logo.
  • Also change the href link of the link from the blog-url and title attribute from blog description

Motivation and context

This PR seeks to solve https://petitions.classicpress.net/posts/165/set-login-page-logo-to-be-site-logo.

How has this been tested?

  1. Install CP core.
  2. Add a logo through the theme customizer.

Screenshots (if appropriate):

Logo added through customizer

Screen Shot 2019-06-19 at 12 18 56

Logo grab automatically and added to the login page

Screen Shot 2019-06-19 at 12 18 30

Source code for the existing logo

Screen Shot 2019-06-19 at 12 19 47

If no logo is available, defaults to CP logo

Screen Shot 2019-06-19 at 12 20 06

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
Add custom theme logo to the login page
* Get the theme logo through customizer.
* Add style with background image if theme logo exists.
 * Also change the href link of the <a> link from the blog-url and title attribute from blog description
src/wp-includes/theme.php Outdated Show resolved Hide resolved
src/wp-includes/theme.php Outdated Show resolved Hide resolved
src/wp-includes/theme.php Outdated Show resolved Hide resolved
@nylen

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

Thanks for the PR. I know it is exploratory but I have done an (also preliminary) review and left some comments above.

The only other thing I saw this time around is that we need to make this new behavior optional. I am not sure whether this option should be enabled or disabled by default.

@bahiirwa

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Thanks for review.

The option of turning it off would mean probably two things.

  1. We can add a custom logo on set up plus an on and off toggle/checkbox.
  2. We would implement this approach above and add an option in the admin settings section. CMS would then make a check to find out if this is set so. Then run the code above.

However from the Request, most of the feedback is to have the custom logo as default and fallback to CP logo if no logo is added. I still see the value of where you come from considering that the CMS has to factor in other needs for different people. If the latter is the case, I would go for option one above.

I will add the tests above and revert.

@nylen

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

I don't think we need to require adding a custom logo on setup. It's a good idea, but that would fit better as a "new user orientation" type of effort (which would be a lot more work than this PR).

We should just disable the option and/or show a warning message if there is no logo set, and I think that will cover it unless I am missing something?

@nylen

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Oh, and, if you run into problems adding tests let me know, I'll be happy to help with that part.

This is something that ClassicPress needs more contributors to learn about eventually though. At the moment the best guide for how to run the tests is here: https://make.wordpress.org/core/handbook/testing/automated-testing/phpunit/

bahiirwa added some commits Jun 26, 2019

Moving edits directly to HTML
Suggestions  to move the login page logo edits to direct HTML.
Moving edits directly to HTML
Suggestion to move the login page logo edits to direct HTML.
Moving edits directly to HTML
Suggestions to move the login page logo edits to direct HTML.

Changes
- Introduce an <img> in the <a>
- Add CSS to give the same feel and look as before with the new image.
@bahiirwa

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

I am trying the PHP Unit tests. However, this is the new direction my PR would take.

src/wp-login.php Outdated Show resolved Hide resolved
*
* @since 1.0.2
*/
$default_logo_img_url = admin_url('/images/w-logo-blue.png?ver=20190218');

This comment has been minimized.

Copy link
@nylen

nylen Jun 28, 2019

Member

This function call needs spaces, and this image is only 80x80px, being displayed in a space that is 84x84px.

How about we use use the SVG image instead?

$default_logo_img_url = admin_url( '/images/wordpress-logo.svg?ver=20190218' );

@bahiirwa you are probably a good person to ask about browser compatibility here, do you ever come across devices that are still running IE 8 or lower? Otherwise, I think this is OK, we have already used SVG images elsewhere in the admin dashboard. Reference: https://caniuse.com/#feat=svg-img

This comment has been minimized.

Copy link
@bahiirwa

bahiirwa Jun 29, 2019

Author Contributor

By default, this particular point of CP/WP takes images about 80px tall and wide.

Will fix the space.

As for devices, not really. IE8 is the default browser for Windows 7 and Windows Server 2008 R2 operating systems however, coming from a continent that is not so forward in tech devices, most windows users run Windows 7 but with other modern browsers like mozilla and chrome.

The pixels disparity might not be a big issue with 4x4px difference. However, we could add a bigger png by default and avoid the guess work. Or take the big leap and go for SVG.

This comment has been minimized.

Copy link
@bahiirwa

bahiirwa Jun 29, 2019

Author Contributor

On the other hand have pushed a css change with width:80px and height:80px

This comment has been minimized.

Copy link
@nylen

nylen Jul 1, 2019

Member

A bigger issue than 80px vs 84px is that 80px is not really 80 pixels on modern devices with high screen density, and each year there are more of these with higher pixel density. Let's go with the SVG here.

This comment has been minimized.

Copy link
@bahiirwa

bahiirwa Jul 3, 2019

Author Contributor

Change effected $default_logo_img_url = admin_url( '/images/wordpress-logo.svg?ver=20190218' ); for hi-res devices.

Could we rename the image to classicpress-logo.svg? Will this have wide ranging issues?
I see some WP Branding infringement is seen here.

This comment has been minimized.

Copy link
@bahiirwa

bahiirwa Jul 15, 2019

Author Contributor

Any comment on the branding rename?

@nylen

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

The new direction for the PR is looking good.

When writing unit tests, we will want to split the new logic out into a new function like get_logo_image_url(). Then the unit tests will just call this function.

If you run into problems setting this up, please let me know, I am happy to help work through that or to help with the test cases myself.

@bahiirwa

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

Thanks for the review.

When writing unit tests, we will want to split the new logic out into a new function like get_logo_image_url(). Then the unit tests will just call this function.

Is there any difference with the_custom_logo() Reference: https://developer.wordpress.org/reference/functions/the_custom_logo/

If you run into problems setting this up, please let me know, I am happy to help work through that or to help with the test cases myself.

I having issues with my composer which might delay the process. Please proceed with tests writing.

@bahiirwa bahiirwa closed this Jun 29, 2019

@bahiirwa bahiirwa deleted the bahiirwa:custom-login-image branch Jun 29, 2019

@bahiirwa bahiirwa restored the bahiirwa:custom-login-image branch Jun 29, 2019

@bahiirwa bahiirwa reopened this Jun 29, 2019

@nylen nylen added this to the v1.1.0 milestone Jul 2, 2019

@bahiirwa

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Any reviews on this?

@nylen

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

This PR has gotten corrupted with some extra commits that shouldn't be in there. Also it looks like the entire file src/wp-admin/css/login.css has been reformatted which is not right.

If you'd like me to try to untangle that, I can, but it probably won't be this week.

@bahiirwa bahiirwa force-pushed the bahiirwa:custom-login-image branch from c727c7d to 836f6c9 Jul 15, 2019

@joyously

This comment has been minimized.

Copy link

commented Jul 15, 2019

I have a problem with how this is being done.
The custom logo option is a core option, but is theme specific, meaning that the theme has to call add_theme_support( 'custom-logo' ) for it to appear in the Customizer. The theme then has control of where it is output on the front end. BUT the theme has nothing to do with the login page (nothing of the theme is loaded there). Any changes to the login page are done in a plugin, and there are already lots of plugins that change the login page.

SO, the way this is coded doesn't make it very backward compatible, and it also doesn't account for multisite. The existing functions has_custom_logo and get_custom_logo both use the $blog_id to get the correct logo for multisite. They also have filters that existing plugins (or themes) would be using.

@Mte90

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

If it isn't backward compatible is the case to put to 2.x, and also without unit tests is not the case to move further.

@bahiirwa

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

This is backward compatible. The issue is using themes to get the information for a core login image. However, this has a fall back of the core stored image in a local directory. It is only when a user adds a theme logo that the core picks the image from customizer as a login image.

Most users interact with the software through the themes so this is an area I think would be a strong point for user experience. We are not replacing the current image but rather giving an option to have a better branding experience for the user.

I have purposeful left the customization intact as the filter hooks allow. So backward changes are possible.

@Mte90

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

@joyously exposed few issues for compatibility also about multisite.

@joyously

This comment has been minimized.

Copy link

commented Jul 16, 2019

This is backward compatible. The issue is using themes to get the information for a core login image.

No, the existing plugins that change the login page work with the existing code, but these changes would not work for them. And using existing functions for getting the custom logo would make it more backward compatible for the plugins that filter that.
The issue not about "using themes to get the information". It is a core option and core function, after all. The issue is that the logo is a theme option and the theme has nothing to do with the login page. The theme is for the front end only. I'm saying the issue is that it is a bit of a conflict to mix theme and plugin functionality this way.

@nylen

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

We are adding a new option that will be disabled by default (this part of the change is not coded yet). If a site uses plugins that modify the login page then they should not enable this option.

So, unless I am missing something, there is no backward compatibility problem here because the default behavior does not change.

I agree we definitely need to be sure this works reasonably in multisite. It sounds like get_custom_logo is a better function to be using here.

@bahiirwa

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

If we use the existing function get_custom_logo(), It has apply_filters embedded in it.

The code below still give us the same output as before but factors in multisite. Would this be the better direction?

function get_logo_image_url() {
	$default_logo_img_url = admin_url( '/images/wordpress-logo.svg?ver=20190218' );
	$custom_logo_id = get_custom_logo(get_current_blog_id());
	if ( empty( $custom_logo_id ) ) {
		$url_image = '<a href="' . esc_url( $login_header_url ) . '" title="' . esc_attr( $login_header_title ) . '" tabindex="-1"><img src="' . $default_logo_img_url . '" alt="' . esc_attr( $login_header_title ) . '" /></a>';
	} else {
		$url_image = $custom_logo_id;
	}
	return $url_image;
}
?>
<div id="login">
	<h1><?php echo get_logo_image_url(); ?></h1>
<?php

unset( $login_header_url, $login_header_title );
@joyously

This comment has been minimized.

Copy link

commented Jul 19, 2019

If we use the existing function get_custom_logo(), It has apply_filters embedded in it.

The code below still give us the same output as before but factors in multisite. Would this be the better direction?

That code is all wrong. get_custom_logo returns markup, not an ID.
What if you add another case to get_bloginfo() for the logo and use that?

@nylen

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

No changes to get_bloginfo please, this function is already a mess.

@bahiirwa

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@nylen and @joyously Could you please help shape this? I have really hit a dead end.

@Mte90

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2019

So @bahiirwa we need to have unit tests also for multisite before to approve this code, in this way we can be sure that everything works and doesn't break anything :-)

@nylen

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

In no particular order:

  • src/wp-admin/css/login.css still needs its spacing restored to use tabs for indentation instead of spaces, this makes the rest of the changes there difficult to read.
  • It sounds like we can use the existing get_custom_logo function to simplify some of the new code added here. This needs to be investigated, including how it works in multisite.
  • We need a new option like cp_login_custom_logo that is set to false by default. When this option is disabled, the markup for the login page should not change (for compatibility with sites that already modify the login screen using other methods). When this option is enabled, and a custom logo is set, then the logic added in this PR can take effect and modify the login page markup.

Then, any new code needs automated tests. Let's save that for last. We are probably not ready to start working on tests yet, we need the basic code structure working well first.

@bahiirwa

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Thanks for the pointers

* `src/wp-admin/css/login.css` still needs its spacing restored to use tabs for indentation instead of spaces, this makes the rest of the changes there difficult to read.

I still think we can add a few lines to the .editorconfig to format the project accordingly. But I have noted a tab indentation of 4.

* It sounds like we can use the existing `get_custom_logo` function to simplify some of the new code added here. This needs to be investigated, including how it works in multisite.

From the docs, https://developer.wordpress.org/reference/functions/get_custom_logo/#source, get_custom_logo does it's own check on multisites and does a get_theme_mod( 'custom_logo' )

However, since get_custom_logo returns its own markup like this:
Screen Shot 2019-08-09 at 22 07 54

One can not directly use the filters like apply_filters( 'login_headerurl', $login_header_url ); or apply_filters( 'login_headertitle', $login_header_title ); that are the current way of making edits to the login form via plugin or functions.php. They would need to make a custom html template that hooks into apply_filters( 'get_custom_logo', $html, $blog_id );.

Making a new function for this section is the best option to keep b/c OR state that when someone chooses to go with this option the old filters do not work.

* We need a new option like `cp_login_custom_logo` that is set to `false` by default. 

I have made a proof of concept here with code that would make that possible.

https://github.com/bahiirwa/ClassicPress/tree/login-image

Screen Shot 2019-08-09 at 22 09 55

that utilizes

if ( get_option( 'cp_login_custom_logo' ) && has_custom_logo() ){ ?>
    <style>
        #login a {
            display: block;
            text-align: center;
        }
    </style>
    <?php echo the_custom_logo(); } else {?>
    <h1><a href="<?php echo esc_url( $login_header_url ); ?>" title="<?php echo esc_attr( $login_header_title ); ?>" tabindex="-1"><?php echo $login_header_text; ?></a></h1>

Is this the new direction we should go? Or should we make a custom function that will allow one to use the filter hooks as before. Basically duplicate get_custom_logo into cp_get_custom_logo and then make custom markup that still allows the filters mention to work.

Then, any new code needs automated tests. Let's save that for last.
Hoping to get help on this.

@nylen

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

I still think we can add a few lines to the .editorconfig to format the project accordingly. But I have noted a tab indentation of 4.

The line that is really necessary is already present: indent_style = tab

The important point here is to avoid automatically reformatting files and introducing unexpected changes, there are still some of these especially in the login.css file. A PR should only change the parts of the code that are relevant to the task at hand.

Making a new function for this section is the best option to keep b/c OR state that when someone chooses to go with this option the old filters do not work.

We should keep these existing filters working. After taking another look at this I agree the best path is to make a new function.

Let's call it get_login_image_html, and it should live in wp-includes/general-template.php. The existing login_headerurl and login_headertitle filters should be moved into this new function.

Expected behavior:

  • If the "use custom logo on login page" option is not set, or there is no custom logo defined, then the outputted HTML should remain the same as it is now: <h1><a href="<?php echo esc_url( $login_header_url ); ?>" title="<?php echo esc_attr( $login_header_title ); ?>" tabindex="-1"><?php echo $login_header_text; ?></a></h1>
  • If the "use custom logo on login page" option is set, and there is a custom logo defined, then the HTML structure should change as needed in order to show the image. The link href should probably default to the site's homepage in this case also.

When all of that is done, we will write tests for the new function by translating those requirements into code. Again don't worry about that for now.

I have made a proof of concept here with code that would make that possible. [...] Is this the new direction we should go?

Any changes in style should be done by adding a new CSS class rather than outputting a <style> tag inline, and we need to make the new function and put it in the right place, but otherwise this latest branch looks good.

The parts of get_custom_logo that we actually need to copy should be minimal, for example, looking at it again we shouldn't need to add any special code for multisite since we always just use the current blog ID.

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