WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 19 months ago

Last modified 19 months ago

#17944 closed enhancement (fixed)

Path not displayed for current theme

Reported by: scribu Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Administration Keywords: needs-patch
Focuses: Cc:

Description

When on wp-admin/themes.php all the themes have a helpful paragraph that goes like this:

All of this theme’s files are located in /themes/some-theme.

Sadly, this bit of information is not displayed for the current theme.

Attachments (19)

17944.diff (1.1 KB) - added by CoenJacobs 3 years ago.
Shows the path of current theme. Shows both parent and current paths when current theme is a child theme.
17944-cleaned-vars.diff (3.0 KB) - added by CoenJacobs 3 years ago.
If unused vars could be removed, the code get's cleaner. Var title has a double anyway.
17944-function.diff (3.2 KB) - added by CoenJacobs 3 years ago.
Moved duplicate code to private function.
17944-standards.diff (3.2 KB) - added by CoenJacobs 3 years ago.
Fixed two Coding Standards issues
17944-docs.diff (5.2 KB) - added by CoenJacobs 3 years ago.
New patch with doc block and remove the underscore for function
template_location.png (59.4 KB) - added by CoenJacobs 3 years ago.
Screenshot of new situation: Theme
template_child_location.png (92.9 KB) - added by CoenJacobs 3 years ago.
Screenshot of new situation: Child Theme
17944-textual.diff (5.2 KB) - added by CoenJacobs 3 years ago.
Textual changes based on previous patch.
17944-new-text-inactive.png (19.1 KB) - added by CoenJacobs 3 years ago.
New text display for inactive child theme.
17944-new-text-active.png (23.5 KB) - added by CoenJacobs 3 years ago.
New text display for active child theme.
17944-textual-2.diff (3.2 KB) - added by CoenJacobs 3 years ago.
New diff, combining all vars to a new single line of text.
17944-new-text-active-2.png (23.7 KB) - added by CoenJacobs 3 years ago.
New text display for active child theme (with patch 2).
17944-new-text-inactive-2.png (50.0 KB) - added by CoenJacobs 3 years ago.
New text display for inactive child theme (with patch 2).
17944-fullscreen.jpg (187.3 KB) - added by CoenJacobs 3 years ago.
Fullscreen screenshot
17944-twentyten.jpg (105.5 KB) - added by CoenJacobs 3 years ago.
Fullscreen screenshot with Twenty Ten active
standalone-theme.png (126.0 KB) - added by CoenJacobs 22 months ago.
New situation with a standalone theme
child-theme.png (135.3 KB) - added by CoenJacobs 22 months ago.
New situation with a child theme
17944-3.4-rewrite.diff (3.2 KB) - added by CoenJacobs 22 months ago.
Updated patch to match new WordPress 3.4 themes screen
17944-remove.diff (1.5 KB) - added by lessbloat 20 months ago.

Download all attachments as: .zip

Change History (57)

CoenJacobs3 years ago

Shows the path of current theme. Shows both parent and current paths when current theme is a child theme.

comment:1 CoenJacobs3 years ago

  • Keywords has-patch ui-feedback needs-testing added; needs-patch removed

CoenJacobs3 years ago

If unused vars could be removed, the code get's cleaner. Var title has a double anyway.

comment:2 CoenJacobs3 years ago

Seems to work perfectly after removing unused vars. Not sure if they should be saved for translations though. If so, first patch is the way to go.

comment:3 scribu3 years ago

  • Keywords ui-feedback removed

You should make a private* helper function, to avoid all the duplication.

*i.e. name is prefixed with an underscore.

Last edited 3 years ago by scribu (previous) (diff)

CoenJacobs3 years ago

Moved duplicate code to private function.

comment:4 CoenJacobs3 years ago

Not quite sure about the way I pass the required theme vars in class-wp-themes-list-table.php (line 176), but this way I can pass a subset of the entire theme array, just what I need. Array keys are with capitals and spaces in that file, so we always need a way to convert them to lowercase and underscore versions (to match the object variabels from wp-admin/themes.php).

comment:5 scribu3 years ago

Looks pretty good, except for the brace style on the method:

http://codex.wordpress.org/WordPress_Coding_Standards#Brace_Style

... and the missing space after the first if.

Last edited 3 years ago by scribu (previous) (diff)

CoenJacobs3 years ago

Fixed two Coding Standards issues

comment:6 CoenJacobs3 years ago

Got that fixed. Thank you scribu!

comment:7 nacin3 years ago

As the method is used outside the class, it should not he underscored. A doc block would also be nice.

Would like a screenshot and UX approval on this. Don't want someone to look at this as unnecessary clutter.

CoenJacobs3 years ago

New patch with doc block and remove the underscore for function

CoenJacobs3 years ago

Screenshot of new situation: Theme

CoenJacobs3 years ago

Screenshot of new situation: Child Theme

comment:8 CoenJacobs3 years ago

  • Keywords ui-feedback ux-feedback added

Added new patch with doc block and removed the underscore for function.

Also added two screenshots with new situation of a theme and a child theme.

comment:9 CoenJacobs3 years ago

Can I get some ui en ux feedback in here? Would be too bad if this doesn't make it through feature freeze, just because it is lost somewhere in the archives. ;)

comment:10 follow-up: jane3 years ago

Can't a child theme add additional templates? That makes the patch text not accurate.

comment:11 in reply to: ↑ 10 CoenJacobs3 years ago

That goes the same for the text that is there right now for the available themes.

comment:12 follow-up: scribu3 years ago

  • Keywords 3.3-early needs-testing ui-feedback removed

Yeah, if we're going to do this, we should change the wording everywhere. One suggestion:

Parent theme files are located in twentyeleven. Child theme files are located in twentyeleven-child.

comment:13 in reply to: ↑ 12 CoenJacobs3 years ago

I'm with scribu here. Changing the lines to a way that is correct in all forms of parent/child theme combinations. I can change my patch to suit the new text.

comment:14 scribu3 years ago

Please do. (Sidenote: no guarantee it will get into 3.3, but at least it will be ready for 3.4).

comment:15 CoenJacobs3 years ago

New patch, new text, new screenshots. All ready for 3.3. ;)

Text for non-child theme stays the same: "All of this theme’s files are located in /themes/twentyten." seems to be pretty good.

Edit: Something clearly went wrong with my previous uploads. Did my trick again and now they're all good.

Last edited 3 years ago by CoenJacobs (previous) (diff)

CoenJacobs3 years ago

Textual changes based on previous patch.

CoenJacobs3 years ago

New text display for inactive child theme.

CoenJacobs3 years ago

New text display for active child theme.

comment:16 follow-up: scribu3 years ago

How about instead of:

Parent theme files are located in /themes/twentyeleven. Child theme files are located in /themes/twentyeleven-child. TwentyEleven Child uses templates from Twenty Eleven.

we shorten it to a single sentence:

TwentyEleven Child (/themes/twentyeleven-child) uses templates from Twenty Eleven (/themes/twentyeleven).

CoenJacobs3 years ago

New diff, combining all vars to a new single line of text.

CoenJacobs3 years ago

New text display for active child theme (with patch 2).

CoenJacobs3 years ago

New text display for inactive child theme (with patch 2).

comment:17 in reply to: ↑ 16 CoenJacobs3 years ago

Some patches got merged on my end, so previous patches had some other changes in them as well. My bad, since patch 17944-textual-2.diff, there are no more problems with that.

Replying to scribu:

TwentyEleven Child (/themes/twentyeleven-child) uses templates from Twenty Eleven (/themes/twentyeleven).

That's a lot better indeed. Worked it out in a new patch + screenshots.

comment:18 scribu3 years ago

  • Milestone changed from Future Release to 3.3

Looks pretty good.

comment:19 follow-up: JohnONolan3 years ago

The alignment on 17944-new-text-active-2.png is slightly strange - I would keep the margin-left created by the theme screenshot and have the text align with the other text above it. Full screenshot rather than crop would be helpful - to get a better idea of information hierarchy.

CoenJacobs3 years ago

Fullscreen screenshot

CoenJacobs3 years ago

Fullscreen screenshot with Twenty Ten active

comment:20 in reply to: ↑ 19 CoenJacobs3 years ago

Replying to JohnONolan:

The alignment on 17944-new-text-active-2.png is slightly strange - I would keep the margin-left created by the theme screenshot and have the text align with the other text above it. Full screenshot rather than crop would be helpful - to get a better idea of information hierarchy.

Added two fullscreen screenshots. I think the alignment is correct this way. To illustrate that, I've added a screenshot of an active Twenty Ten install, where the new paragraph is aligned next to the screenshot. Here you can see that the margin is the same on both sides.

comment:21 JohnONolan3 years ago

Yep - you're right, I take it back. Looks pretty good to me, only other thing that stands out to me personally is that I would probably opt to say "uses template files from /xxx/" rather than "uses templates from /xxx/" - but Jane will be able to offer better insight on that one as she has a far wider knowledge of the nuances between terms like this throughout the rest of core.

comment:22 ocean902 years ago

  • Type changed from defect (bug) to enhancement

comment:23 nacin2 years ago

  • Milestone changed from 3.3 to Future Release

comment:24 CoenJacobs2 years ago

With all the new improvements for themes, this looks like a nice little enhancement for 3.4.

Patch is all done. Only need UI/UX feedback and the text might need a little look at.

Last edited 2 years ago by CoenJacobs (previous) (diff)

comment:25 SergeyBiryukov2 years ago

  • Keywords 3.5-early added

comment:26 CoenJacobs22 months ago

  • Keywords ui-feedback added

Right, this is 3.5-early period, so I'm not gonna let this one slip another version. :)

We might need to reconsider the text, but UI/UX feedback shouldn't be too hard to get, right?

comment:27 scribu22 months ago

  • Keywords ux-feedback 3.5-early removed
  • Milestone changed from Future Release to 3.5

comment:28 nacin22 months ago

We now hide tags, and bury file locations inside a 'Details' link by default.

I've always thought of these file paths as potentially useful but ultimately extraneous information for the vast majority of users, not to mention an eyesore. Do we honestly need them? I'm not decreeing "No," but I'm suggesting we need to really think about it.

As a practical matter, this needs to be updated to use WP_Theme.

Secondarily, there is the potential that a parent theme is invalid for the current theme on disk (and in WP_Theme) but not for what is the current 'template' option in the DB. This makes for fun failures: 20921#comment:1.

CoenJacobs22 months ago

New situation with a standalone theme

CoenJacobs22 months ago

New situation with a child theme

comment:29 CoenJacobs22 months ago

Thanks nacin, I have updated the patch to the way the themes screen now works. Also added two screenshots of a standalone theme and a child theme.

Does this patch need to check for the potential invalid theme, or is that something we need to catch elsewhere?

CoenJacobs22 months ago

Updated patch to match new WordPress 3.4 themes screen

comment:30 lessbloat22 months ago

I'm new around these parts, sorry for just jumping in here near the end of the thread.

Two quick thoughts:

1) My gut says that 80% of WP users would not find this information useful. I have no way to quantify this guess, it's just a hunch. With that said, I'd almost prefer that we not add it.

2) If we do decide to add it, can we:

  • Remove the "Twenty Eleven child uses templates from Twenty Eleven. Changes made to the templates will affect both themes." lines when a child theme is selected.
  • Transition the "All of this theme's files are located in: themes/twentyeleven" line up to the end of the monster description paragraph above.

comment:31 ryan21 months ago

I too would rather remove the locations.

lessbloat20 months ago

comment:32 lessbloat20 months ago

17944-remove.diff simply removes the locations.

comment:33 nacin20 months ago

I think this information is useful in one way only: It is the only way in the UI (outside of the theme editor) to identify that a theme is a child theme of another theme.

The question is, useful for whom? Is this useful for few, some, or most users?

If we determine it is useful information, we can simply include a line "Child theme of %s." That's, at most, what I'd like to put here.

comment:34 nacin20 months ago

In [21650]:

Remove paths to where theme files are located from theme details on themes.php. props lessbloat, see #17944.

comment:35 scribu20 months ago

+1 for "Child theme of %s". Users that have access to that screen need to understand that relationship, lest they go about deleting the parent theme.

comment:36 helenyhou20 months ago

  • Keywords needs-patch added; has-patch ui-feedback removed

Another +1 for "Child theme of %s"

comment:37 nacin19 months ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [21816]:

Indicate on themes.php when a theme is a child that requires a parent theme. fixes #17944.

comment:38 nacin19 months ago

In [21817]:

Undo nested paragraphs added in [21816]. see #17944.

Note: See TracTickets for help on using tickets.