WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 3 months ago

#46925 closed enhancement (fixed)

Site Health: Update 'Other Themes' to 'Inactive Themes' and give 'Parent Theme' it's own accordion.

Reported by: garrett-eclipse Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: has-screenshots site-health has-patch commit
Focuses: ui Cc:
PR Number:

Description

Hello,

Currently, activating a child theme lists it on the 'Active Theme' accordion and leaves the parent theme in the 'Other Themes' list. I was originally seeing the 'Other Themes' list as the inactive list so could see it being confused.

It would be nice to list the parent either in it's own 'Parent Theme' section or along with the child making the accordion 'Active Themes' and updating the 'Other Themes' to be 'Inactive Themes' to match the status test for 'You should remove inactive themes'.

Thoughts?
Thanks

Attachments (9)

Screen Shot 2019-04-14 at 1.10.23 PM.png (253.0 KB) - added by garrett-eclipse 6 months ago.
Currently, Parent theme is showing in 'Other Themes' section
46925.diff (3.2 KB) - added by garrett-eclipse 6 months ago.
Patch to introduce 'Parent Theme' accordion and rename 'Other Themes' to 'Inactive Themes'
Screen Shot 2019-04-14 at 1.39.07 PM.png (272.9 KB) - added by garrett-eclipse 6 months ago.
Parent Theme section similar to Active Theme but without 'Parent theme' and 'Theme features' rows.
46925.2.diff (3.2 KB) - added by garrett-eclipse 6 months ago.
Updated patch to address the warnings caused by use of non-existent $parent_theme_updates
Screen Shot 2019-04-15 at 10.43.41 PM.png (138.9 KB) - added by garrett-eclipse 6 months ago.
Updated Active and Parent Theme sections w/ slugs
46925.3.diff (4.6 KB) - added by garrett-eclipse 6 months ago.
Refreshed patch to introduce slugs to active theme name, parent theme name and the parent theme found in the active theme section
46925.4.diff (5.1 KB) - added by xkon 6 months ago.
46925.5.diff (5.0 KB) - added by garrett-eclipse 6 months ago.
Minor update to cleanup the exclusion checks
46925.6.diff (5.0 KB) - added by garrett-eclipse 5 months ago.
Refreshed for 5.3

Download all attachments as: .zip

Change History (25)

@garrett-eclipse
6 months ago

Currently, Parent theme is showing in 'Other Themes' section

@garrett-eclipse
6 months ago

Patch to introduce 'Parent Theme' accordion and rename 'Other Themes' to 'Inactive Themes'

@garrett-eclipse
6 months ago

Parent Theme section similar to Active Theme but without 'Parent theme' and 'Theme features' rows.

#1 @garrett-eclipse
6 months ago

  • Keywords has-patch needs-testing has-screenshots added

I've uploaded 46925.diff to introduce a Parent Theme accordion section similar to Active Theme, and to rename the 'Other Themes' to 'Inactive Themes' to match the Info tests.

The Parent theme row isn't present in the Parent Theme section on purpose. As to the Theme Features as the Active Theme section retrieved that information from a global option it wasn't easy to do the same for the Parent Theme so omitted it as it's basically covered in the Active Theme anyway and would just be duplicate information.

#2 @mukesh27
6 months ago

  • Keywords site-health added

#3 @mukesh27
6 months ago

@garrett-eclipse tested above patch and found below errors.

Notice: Undefined variable: parent_theme_updates in /var/www/html/wordpress/wp-admin/includes/class-wp-debug-data.php on line 913

Warning: array_key_exists() expects parameter 2 to be array, null given in /var/www/html/wordpress/wp-admin/includes/class-wp-debug-data.php on line 913

@garrett-eclipse
6 months ago

Updated patch to address the warnings caused by use of non-existent $parent_theme_updates

#4 @garrett-eclipse
6 months ago

Thanks @mukesh27 I appreciate the catch there. Uploaded 46925.2.diff to address these issues. They're both simply related to my rename of the $theme_updates accidentally to $parent_theme_updates when I was updating the other variables I carried over for use int he parent theme accordion.

Can you give it a re-test and let me know if you have any further issues.

Cheers

#5 @Clorith
6 months ago

  • Keywords has-patch removed
  • Milestone changed from Awaiting Review to 5.3

The parent theme row under "Active theme" should probably show the slug as well, and the slug should also be shown in the parent theme section (this is one of the most common reasons why a parent theme isn't working I've noticed, they use the wrong slug, or try to use the name instead of the slug).

I see the parent theme is also missing its own slug, which would be good to include there somehow.

Beyond that, I think this is a good enhancement for 5.3

@garrett-eclipse
6 months ago

Updated Active and Parent Theme sections w/ slugs

@garrett-eclipse
6 months ago

Refreshed patch to introduce slugs to active theme name, parent theme name and the parent theme found in the active theme section

#6 @garrett-eclipse
6 months ago

  • Keywords has-patch added

Thanks @Clorith I appreciate the feedback and the milestoning.

I've updated the patch (46925.3.diff) to follow the convention from the other themes list to include the slug in parentheses which I've applied to the 'Name' field on both the active and parent sections as well as the 'Parent theme' field in the Active theme section.

Updated Debug Data;

### wp-active-theme ###

name: Honeybadger ChildTheme (honeybadger-child-master)
version: 1.0
author: XXXXX
author_website: http:// www. xxx .ca
parent_theme: Honeybadger (honeybadger-theme)
theme_features: menus, post-thumbnails, automatic-feed-links, title-tag, html5, post-formats

### wp-parent-theme ###

name: Honeybadger (honeybadger-theme)
version: 0.1.10
author: XXXXX
author_website: https:// www. xxx .ca

Good to go for testing here.
Cheers

@xkon
6 months ago

#7 @xkon
6 months ago

This is a nice addition thanks @garrett-eclipse !

46925.4.diff is based on 46925.3.diff .

Additions & changes:

1) Fixes some minor CS issues.

2) Changes

if ( $active_theme->stylesheet === $theme_slug || $parent_theme->stylesheet === $theme_slug ) {

to

if (
    $active_theme->stylesheet === $theme_slug ||
    ! empty( $parent_theme->stylesheet ) && $parent_theme->stylesheet === $theme_slug
) {

To avoid the Trying to get property 'stylesheet' of non-object error if a parent theme is selected.

3) Converts the Theme directory location of the Active Theme to use get_stylesheet_directory() instead.

4) Adds the Theme directory location to the Parent Theme as well with get_template_directory()

@garrett-eclipse
6 months ago

Minor update to cleanup the exclusion checks

#8 @garrett-eclipse
6 months ago

Thanks @xkon I always appreciate the CS fixes. And for catching the non-object check, as well as the theme_path updates. This looks pretty good to go, maybe some more testing and we'll see if it needs a refresh when 5.3 rolls around.

P.S. I did make one update to the $all_themes exclusion. I was updating the preceding comment and felt it starting to cluster so I've separated the two exclusion check giving them each their own comment. 46925.5.diff

#9 @xkon
5 months ago

@Clorith could you check the latest patch here in case we missed something so we can get this in maybe?

@garrett-eclipse
5 months ago

Refreshed for 5.3

#10 @msaggiorato
4 months ago

I just tested this as part of my first ever core contribution, and it seems like the patch does what it needs to be doing.

It would be nice if add_theme_support actually used backtrace or something of the sort, to know which theme (child or parent) adds a specific feature support. But that's not related to this ticket.

#11 @desrosj
4 months ago

  • Component changed from Administration to Site Health

Moving Site Health tickets into their lovely new home, the Site Health component.

#12 @garrett-eclipse
4 months ago

Thanks @msaggiorato for testing and I agree that backtrace would be nice, do you mind opening another ticket here on Trac for that suggestion. Cheers

#13 @Clorith
3 months ago

  • Keywords commit added; needs-testing removed

46925.6.diff looks good, I like the approach, and the re-labeling of Other themes to Inactive themes makes much more sense from a reading perspective.

#14 @SergeyBiryukov
3 months ago

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

#15 @garrett-eclipse
3 months ago

  • Summary changed from Site Health: Should the parent theme be listed in 'Active Theme' on info tab? to Site Health: Update 'Other Themes' to 'Inactive Themes' and give 'Parent Theme' it's own accordion.

*Updated title to provide a clear description of this tickets current direction for easier review

#16 @SergeyBiryukov
3 months ago

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

In 45680:

Site Health: Show parent theme in its own accordion on Site Health Info screen; rename "Other Themes" to "Inactive Themes".

Props garrett-eclipse, mukesh27, Clorith, xkon, msaggiorato.
Fixes #46925.

Note: See TracTickets for help on using tickets.