Accepting contributions?

[HardeepAsrani]

Is this theme, as well as Template, accepting contributions? Saw some things that don't comply with best practices for themes, so wanted to send a PR.

[joshuaiz]

Yes!

[HardeepAsrani]

@joshuaiz Should I make a review of things before I commit so you know what's wrong and what's the things that you wanted to like that. Like favicon feature is in WordPress core so you shouldn't add it to the theme at all. Let me know

[joshuaiz]

Yes go ahead.

…

On Oct 28, 2017, at 6:09 PM, Hardeep Asrani ***@***.***> wrote: @joshuaiz Should I make a review of things before I commit so you know what's wrong and what's the things that you wanted to like that. Like favicon feature is in WordPress core so you shouldn't add it to the theme at all. Let me know — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[joshuaiz]

Just following up on this - do you have any suggestions? Would love to see them.

Regarding the favicon, while it is in WP core, it is an extra step to have to set that with every site so I would rather have a default in header.php.

And just because it is in core, doesn't mean the old way doesn't work — I still create the favicons myself and add them to the theme. All that said, I could see the argument for keeping it outside the theme as it isn't theme-dependent.

[HardeepAsrani]

@joshuaiz Thanks for reminding. It's been in my ToDo for a while but I'm procrastinating a lot, honestly, so been skipping.

And yes, that's the argument for favicon. You don't change your Favicon or logo with your theme, it's something which shouldn't be part of the theme.

Let me review your theme right now before I procrastinate again.

[joshuaiz]

@HardeepAsrani While I get the argument for separation, at least for my studio, we do design and development so we are often creating not only the logo, but the favicon and the theme. As hard as we try to push clients to maintain and update their site(s) themselves, it is often a losing battle.

Thus, using the core customizer function for favicon or even site logo really depend on the level of client involvement. If we (my studio) is handling everything, I would rather do it programmatically rather then through the WP Customizer or Site Options.

More broadly though, this should have been implemented long ago in core. Yet, now that it is an option, even when clients use this we still often need to clean it up and style it. Sometimes it's better to just handle it in the theme and not even let the client know it is there.

As with many things, having two ways to do something isn't necessarily bad and while I agree that the site icon is site-independent, I would still leave the favicon call in with Plate. It can be removed easily enough.

We use the customizer much more (including logos and banners) in our sister theme Template: https://github.com/joshuaiz/template

Maybe you want to check out that as well.

Lastly, these things depend on the particular client and the project. Some developers will want/need to handle this themselves and doing it programmatically gives them more flexibility while using the built-in WP functions make it easier on the client.

So, I wouldn't remove it from the theme but I will add some comments about the Customizer and Site Options.

Anything else I'd be happy to review.

[HardeepAsrani]

1. Favicon option shouldn't be a part of the theme, as we noted above. If someone is trying to use this starter theme to make a template for WPorg or Envato, it's against the guidelines to hardcode such things.

2. Pingback should only be loaded if the it's a singular page/post && pingbacks are open, so instead of:

<link rel="pingback" href="<?php bloginfo('pingback_url'); ?>">

It should be:

<?php if ( is_singular() && pings_open( get_queried_object() ) ) : ?>
<link rel="pingback" href="<?php bloginfo( 'pingback_url' ); ?>">
<?php endif; ?>

3. Parts in header.php, such as:

<?php // put font scripts like Typekit here ?>
<?php // drop Google Analytics Here ?>

Should be removed as user should use enqueue scripts/styles/fonts using functions.php file. And just like logo and favicon, your Google Analytics code is not theme-dependent and should be added using a plugin.

4. Instead of logo being added like this:

<?php echo get_theme_file_uri(); ?>/library/images/logo.png" />

It should be added like this: https://developer.wordpress.org/themes/functionality/custom-logo/

5. You shouldn't hardcode alt text or any text anywhere:

<img class="header-image" src="<?php header_image(); ?>" alt="Header graphic" />

You can get it from the image being used. And it should also be translatable.

6. Charset shouldn't be hardcoded like this:

<meta charset="utf-8">

Instead this should be used:

<meta charset='<?php bloginfo( 'charset' ); ?>'>

7. Honestly, you can just use this for the head part:

<!DOCTYPE html>
<html <?php language_attributes(); ?>>
<head>
<meta charset='<?php bloginfo( 'charset' ); ?>'>
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="profile" href="http://gmpg.org/xfn/11">
<?php if ( is_singular() && pings_open( get_queried_object() ) ) : ?>
<link rel="pingback" href="<?php bloginfo( 'pingback_url' ); ?>">
<?php endif; ?>
<?php wp_head(); ?>
</head>

It will work just fine and without any issues. And yes, the header should have rel="profile".

8. Don't use get_stylesheet_directory_uri function, but use get_template_directory_uri instead, else it won't load with a child theme.

9. Don't define protocol as http for Google Fonts. It's better to just use // instead.

10. For enqueuing styles/scripts, don't prefix third-party libraries. If the library is without any changes, like modernizr, you don't need to prefix it. This is to avoid multiple plugins/themes loading the same library.

11. You don't need to use wp_register_script but just use wp_enqueue_script instead. Also, add jQuery as a dependency instead of loading it in the end as it might raise issues once people start using the theme to edit.

12. Instead of using $wp_styles->add_data, you can use this function: https://developer.wordpress.org/reference/functions/wp_script_add_data/

13. This isn't needed add_theme_support( 'menus' );

14. WooCommerce support should be conditional, only if WC is installed.

[joshuaiz]

All these are great. Let me look after the new year and respond in kind.

Yes some of the legacy Bones stuff are either pre- or just after WP 3.X so it has been outdated.

One thing I noticed off the bat is that get_theme_file_url(); has replaced get_template_directory_uri(); as of WP 4.7 so we have already implemented this change.

Maybe download the latest updates (as of yesterday) and have another look.

All that said, these suggestions are super welcome and thanks so much!

[joshuaiz]

@HardeepAsrani Just going over a few things briefly:

1. This theme was designed as a starter theme for our studio. If it is going to be used elsewhere as the basis of another premium theme/project, that's fine but then they can remove the bits they don't like. I don't think the favicon should be removed completely (but it can be for your project).

2. Ok will implement this...good catch.

3. I have to disagree. While GA may be theme-independent, I don't see the advantage of using a plugin other than if you know you are going to change themes and developers. For past projects, I was using one and then it got abandoned and stopped working altogether. So, what do you do then? It's easy enough to drop GA code in the header and it takes 2 seconds. Any dev worth their salt will know to add this in header.php. If a new developer takes over your project and develops a new theme, they can just copy/paste in 2 seconds and then you're done. Way, way easier than adding and updating a plugin that may become obsolete.

4. We have both options: add logo via customizer or via code. Developers don't always want to use the customizer so I don't think this is a bad thing. This is how it was done before the WP Customizer. That there is both is fine, but we shouldn't exclude one or the other. Both are valid.

Our Template theme has more support for this and customizer in general — it is more client based. Plate is really targeted towards developers.

5. Yes you are right - will implement.

6. Yes you are correct - will implement.

7. Let me go over what you have and what we have now. I'm all for reduction.

8. This is now: get_theme_file_url();

9. Yes you are correct. updated

10. Prefix removed.

11. I thought scripts needed to be registered before being enqueued but if that is not necessary, I will remove it. It used to be required. And yes, I removed the jQuery enqueue in the latest commit as it wasn't necessary.

12. In the docs (https://developer.wordpress.org/reference/functions/wp_script_add_data/) this seems to only apply to scripts, not styles. Do you have a reference to show this works for styles as well? I've left this as-is for now.

13. Another holdover from Bones...will remove. removed

14. Yes will add conditional for the WC function. added

Thanks again - these are great!

[joshuaiz]

One more thing about Typekit - enqueueing Typekit fonts is not as efficient as using their javascript and it should be in the <head>. If you want to enqueue in functions.php, you can but placing their js code in the header is faster and avoids FOUC in my experience.

In any event, the space is there in header.php - you don't have to use it.

[joshuaiz]

@HardeepAsrani Just pushed up a lot of your suggestions and my previous post replying to your points has been edited.

Again, thanks for these suggestions - really helpful.

[joshuaiz]

Going to close this for now. @HardeepAsrani (or anyone) if you have a specific enhancement or recommendation, create a new issue. Thanks!