WordPress.org

Make WordPress Themes

Opened 3 years ago

Closed 2 years ago

#24205 closed theme (not-approved)

THEME: Source – 0.0.8

Reported by: KalenJohnson Owned by: mindctrl
Priority: new theme Keywords: theme-source
Cc: info@…

Description

Change History (26)

#1 @themetracbot
3 years ago

  • Owner set to mindctrl
  • Status changed from new to reviewing

#2 @mindctrl
3 years ago

Hi Kalen,

I've completed an initial review of your theme. Here are my findings.

  • Theme includes WPupdatePHP library and requires PHP 5.4. This is something I have to check with other reviewers about. WordPress still supports 5.2.4 unfortunately, so I need to find out whether or not themes in the directory are allowed to drop support for it. I'll let you know my findings.
  • In lib/assets.php, you're loading Google fonts over http. This will cause issues on https sites. Changing it to https will ensure it works great on all sites.
  • Your documentation is in the markdown format. The directory only reads readme.txt files. Can you convert it?
  • root_relative_url and url_compare in lib/utils.php aren't being used and can be removed.
  • Add support for "more" tag (a link or button or something) http://f.cl.ly/items/2D0z2w1n3z2w103W3N3Z/Image%202015-04-23%20at%205.07.48%20PM.png
  • Opening the sidebar causes the admin bar to be hidden - http://f.cl.ly/items/0k1N1g3y1b0U030j3o2h/Image%202015-04-23%20at%204.33.25%20PM.png
  • Page titles don't show in search results
  • When viewing a specific comment (see URL in screenshot), opening the menu causes a weird visual problem. http://f.cl.ly/items/3L2K1m1u1Z2a3P2X2Z06/Image%202015-04-23%20at%204.31.08%20PM.png
  • Navigating 'back' from a page or post results in the scroll position always being returned to the top of the page. Would be nice if this didn't happen.
  • Posts with no title have no link to go to the single view. http://f.cl.ly/items/0h2M0R3h1S2u141J3U2J/Image%202015-04-23%20at%204.41.56%20PM.png
  • Content flows outside the TinyMCE editor window. http://f.cl.ly/items/3g3b3z1S460U2q07183r/Image%202015-04-23%20at%204.45.05%20PM.png

#3 @mindctrl
3 years ago

Kalen,

I have confirmed that as long as your theme fails gracefully on older versions of PHP, and it appears to do so, that it will be allowed in the directory.

#4 @KalenJohnson
3 years ago

Thanks for the quick review @mindctrl, I'll have some fixes up later today

#5 @mindctrl
3 years ago

Thanks, Kalen. I forgot to note that all custom functions, variables, options, etc. must be prefixed with the theme name. I think you have most things namespaced, but wanted to mention it just in case. That's listed on the requirements page.

Also please wrap WPUpdatePhp.php in a class_exists check to prevent any fatal errors if some other plugin is using the same library.

#6 @KalenJohnson
3 years ago

Thanks mindctrl, yes the namespaces are being used instead of prefixing.

I'll be pushing up the fixes today.

#7 @themetracbot
3 years ago

  • Summary changed from THEME: Source – 0.0.1 to THEME: Source – 0.0.2

#8 @themetracbot
3 years ago

  • Summary changed from THEME: Source – 0.0.2 to THEME: Source – 0.0.3

#9 @mindctrl
3 years ago

Hey Kalen, thanks for the update. Most of these things have been fixed up. I see a few remaining things.

I was able to test the theme on PHP 5.2.17 (ugh I know), and the theme fatal errors because the PHP 5.2.* doesn't support the array short syntax you're using in functions.php, lib/php-check.php, and lib/wrapper.php.

I get this error:
Fatal error: syntax error, unexpected '[' in /wp-content/themes/source/functions.php on line 20

Can you fix those up with the array() syntax instead?

The Font Awesome fonts are throwing a 404 error now due to the path being wrong - double styles folder in the path.
Failed to load resource: the server responded with a status of 404 (Not Found) /wp-content/themes/source/dist/styles/styles/fonts/fontawesome-webfont.woff?v=4.3.0

The <!--more--> more tag still isn't supported.

And the visual glitch that happens when opening the menu when directly viewing a comment link.
http://f.cl.ly/items/33212X1Y1C1j2P3S3C1i/Image%202015-05-14%20at%203.21.46%20PM.png

#10 @KalenJohnson
3 years ago

Hey mindctrl,

I'm about to upload the fix for the PHP error and the 404 on the fontawesome files.

I'm not so sure about the more tag, it seems like the only way to enable support for the <!-- more --> tag is to use the_content(), but then that means that any post that does not use that tag will display the full content. I'm not really a fan of that. Seems like we should be able to use the_excerpt(), if the more tag is a requirement then it should be handled by the_excerpt().

Also, I'm not seeing the comments issue, but I'll bump the version number of the css to make sure it's not a cache issue.

#11 @themetracbot
3 years ago

  • Summary changed from THEME: Source – 0.0.3 to THEME: Source – 0.0.4

#12 @themetracbot
3 years ago

  • Summary changed from THEME: Source – 0.0.4 to THEME: Source – 0.0.5

#13 @KalenJohnson
3 years ago

I had noticed there were short array syntax in the php-check.php file too. Whoops, that certainly won't work....

#14 @mindctrl
3 years ago

Thanks for the update, Kalen. It looks like we missed a short array syntax in lib/init.php in register_nav_menus and in register_sidebar.

But now I'm also getting another error in PHP 5.2.17 that I wasn't getting before, presumably because I'm getting further in the load process now:

PHP Notice:  Use of undefined constant __DIR__ - assumed '__DIR__' in /wp-content/themes/source/vendor/autoload.php on line 5

PHP Warning:  require_once(__DIR__/composer/autoload_real.php) [<a href='function.require-once'>function.require-once</a>]: failed to open stream: No such file or directory in /wp-content/themes/source/vendor/autoload.php on line 5

PHP Fatal error:  require_once() [<a href='function.require'>function.require</a>]: Failed opening required '__DIR__/composer/autoload_real.php' (include_path='.:/Applications/MAMP/bin/php/php5.2.17/lib/php') in /wp-content/themes/source/vendor/autoload.php on line 5

#15 @themetracbot
3 years ago

  • Summary changed from THEME: Source – 0.0.5 to THEME: Source – 0.0.6

#16 @KalenJohnson
3 years ago

Hey mindctrl, thanks for bearing with me. I tried using phpenv in a VM to get PHP 5.2 installed... And failed. So basically I've just wrapped all the actual theme code in the if statement if PHP 5.4 is installed. Should call gracefully now.

#17 @KalenJohnson
3 years ago

Hey @mindctrl, just checking in.

#18 @mindctrl
3 years ago

@kalenjohnson sorry, I missed the notification from your last update. I'll try to check this out tonight and get back with you. Thanks for pinging me.

#19 @mindctrl
3 years ago

@kalenjohnson thanks for your patience.

I checked out the updates. On PHP 5.2.17, if you try to Live Preview the theme before activating it, the preview window just shows a spinner with no indication that it won't work. If you Save and Activate, it returns to the front end and white screens. If you reload the white screen, the default theme ends up loading, but I don't think the review admins will let it pass like this. Can show a notice in the customizer too?

The admin notice says "Unfortunately, this *plugin* can not run" - can you change that to "theme"? Related to that string, it's missing a text domain in the library you're using.

You're registering a primary_navigation nav menu but not calling it anywhere. I didn't notice this before (could have missed it). Did you remove it?

One other thing I missed in my previous review was that there's no post navigation. The Theme Check plugin reports: REQUIRED: The theme doesn't have post pagination code in it. Use posts_nav_link() or paginate_links() or the_posts_pagination or next_posts_link() and previous_posts_link() to add post pagination.

Can you fix that up so it'll pass?

#20 @mindctrl
2 years ago

@kalenjohnson just checking in to see if you got my last message.

#21 @KalenJohnson
2 years ago

Hey mindctrl, yes thanks for sticking with me... Let this go for a little too long.

So with the Live Preview button, I'm sort of at a loss about what to do here. I don't see any hooks, filters, callbacks, etc. for disabling or not allowing a Live Preview for a theme. If PHP 5.2 support isn't required, I'm not fully sure how else to handle this particular situation. Perhaps you're just the most thorough reviewer ;)

For the pagination code, not sure why that would be showing up, it is https://github.com/kalenjohnson/source-theme/blob/master/index.php#L14-17

I will upload a new version shortly with the text and nav menu fixes.

#22 @themetracbot
2 years ago

  • Summary changed from THEME: Source – 0.0.6 to THEME: Source – 0.0.7

#23 @themetracbot
2 years ago

  • Summary changed from THEME: Source – 0.0.7 to THEME: Source – 0.0.8

#24 @mindctrl
2 years ago

Kalen,

Thanks for the update. With this latest update, I'm seeing the "Open Menu" text clashing with the hamburger icon. Are you seeing this?

http://f.cl.ly/items/1z2i0I3M0q1i1N1F1D2b/Image%202015-08-06%20at%203.21.47%20PM.png

With regards to the customizer preview piece, I'll have to let one of the other reviewers chime in on that.

#25 @KalenJohnson
2 years ago

Hey, yes I saw that issue too, had it fixed in an update but will double check.

I also had a talk with Otto about the whole PHP issue, basically his suggestion was to not switch themes, but instead if PHP is under 5.4, create a template and filter it to always show that, which would just show "PHP 5.4 is required" or something like that. So it can be activated but will just not display anything other than that.

I'll try and have that up this weekend.

#26 @karmatosed
2 years ago

  • Resolution set to not-approved
  • Status changed from reviewing to closed

I am closing this ticket as you've not responded to the review. In future please respond, even to say you need more time within 7 days.

Note: See TracTickets for help on using tickets.