WordPress.org

Make WordPress Core

#27708 closed defect (bug) (fixed)

Theme installer theme previewing fixes

Reported by: nacin Owned by: matveb
Milestone: 3.9 Priority: high
Severity: minor Version: 3.9
Component: Themes Keywords: has-patch
Focuses: Cc:

Description

  1. theme-install.php?theme=twentyten route doesn't open the overlay. (High priority.)
  1. The "Preview" button should still show for already installed themes. (Low priority.)

We can add minor things here as necessary. Bigger things should end up on their own ticket.

Attachments (4)

27708.1.diff (3.4 KB) - added by matveb 15 months ago.
27708.2.diff (3.4 KB) - added by matveb 15 months ago.
27708.3.diff (7.3 KB) - added by matveb 15 months ago.
27708.4.diff (8.3 KB) - added by matveb 15 months ago.

Download all attachments as: .zip

Change History (20)

comment:1 @nacin15 months ago

In 28038:

Theme Installer: Show 'Preview' button for installed themes.

props adamsilverstein.
see #27055, see #27708.

comment:2 @nacin15 months ago

[28038] handles point 2. The theme screenshot is still clickable, but it doesn't get a pointer cursor, a transparent overlay, or a "Details & Preview" label. I actually kind of like it as-is. It would still be able to receive an "Enter" event, for example. Will let matveb review.

comment:3 @nacin15 months ago

  • Severity changed from normal to minor

comment:4 @afragen15 months ago

Possibly related. This is my first venture into reporting so please be gentle. ;)

I think I'm running across a similar error in hooking into 'themes_api' filter.

I found that load-styles.php:11377 is adding

.install-theme-info { display: none; }

overriding with a display: block; using 'admin_head' filter solves issue.

Quite possibly related to fact that my call to themes_api filter does not have a valid URI in $response->preview_url

I believe this parameter only works for URI in wp.org repo, my parameter has text only.

Last edited 15 months ago by afragen (previous) (diff)

comment:5 follow-up: @nacin15 months ago

What are you doing with the themes_api filter? We are not actually triggering that filter as of 3.9.

comment:6 in reply to: ↑ 5 @afragen15 months ago

Replying to nacin:

What are you doing with the themes_api filter? We are not actually triggering that filter as of 3.9.

I'm using it to display info for the "View version xxx details" overlay. I'm using this in my GitHub Updater plugin for theme updates. If it's not currently triggering in 3.9 something seems to be happening. I'm not explaining this well.
Here's the code I'm using.

@matveb15 months ago

@matveb15 months ago

comment:7 @matveb15 months ago

Patch above sends a request for the theme slug passed in ?theme=themename.

It doesn't yet trigger the preview — it would be as simple as triggering a click on the element — because it's interacting weirdly with pagination. Should disable pagination altogether if only one result.

comment:8 @matveb15 months ago

Also, the data structure of the theme returned for these requests is slightly different — the description doesn't map well for instance.

comment:9 @nacin15 months ago

  • Keywords has-patch added
  • Owner set to matveb
  • Status changed from new to assigned

Patch seems fine for 3.9 as-is. Not opening the preview is not a big deal. Can we make sure "Featured" isn't "chosen"?

comment:10 @nacin15 months ago

I tried to drop in a $( '.theme-filter, .theme-section' ).removeClass( 'current' ); but the delay of 300ms seems to be to try to let the featured API call come back first? If it takes longer than that (or something), I end up with the "Featured" section not only selected, but rendered too. This seems like a super hack.

comment:11 @nacin15 months ago

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

In 28105:

Theme install: Set a timeout and error message for API responses.

props ocean90.
fixes #27708.

comment:12 @nacin15 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[28105] was meant for #27738.

comment:13 @matveb15 months ago

Haven't found an easy way to go around this with the way we've set it up — the same happens with a search route.

To handle this properly I think the best is to:

  • Move listeners and non-sorting code out of the browse method and into render().
  • Handle a browse/sort route the same way as we’ll handle search and theme.
  • Set up a general event route for a vanilla url that triggers a Featured sort.

browse was set up this way because we were bootstrapping data initially, but now it's a regular request, so it should be separate. I may be over thinking it but wasn't able to work around this.

@matveb15 months ago

comment:14 @matveb15 months ago

Patch .3 should finish this:

  • Set up app exclusively via routes which gives us more flexibility: ?theme and ?search no longer clash with featured.
  • Maps description attribute for theme_information queries so that description is available on preview (maybe the API can be updated instead).
  • Cleans up routes, sort/browse are aliases.
  • Completely hides prev/next navigation if the collection only has one item.

I worked on this offline on the plane, and I'm still in transit, so I could only briefly test with real api requests. Please make sure things are working fine.

comment:15 @ircbot15 months ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

@matveb15 months ago

comment:16 @nacin15 months ago

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

In 28123:

Theme installer: Improve route handling and make ?theme= work.

props matveb.
fixes #27708.

Note: See TracTickets for help on using tickets.