Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#27708 closed defect (bug) (fixed)

Theme installer theme previewing fixes

Reported by: nacin's profile nacin Owned by: matveb's profile 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 11 years ago.
27708.2.diff (3.4 KB) - added by matveb 11 years ago.
27708.3.diff (7.3 KB) - added by matveb 11 years ago.
27708.4.diff (8.3 KB) - added by matveb 11 years ago.

Download all attachments as: .zip

Change History (20)

#1 @nacin
11 years ago

In 28038:

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

props adamsilverstein.
see #27055, see #27708.

#2 @nacin
11 years 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.

#3 @nacin
11 years ago

  • Severity changed from normal to minor

#4 @afragen
11 years 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 11 years ago by afragen (previous) (diff)

#5 follow-up: @nacin
11 years ago

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

#6 in reply to: ↑ 5 @afragen
11 years 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.

@matveb
11 years ago

@matveb
11 years ago

#7 @matveb
11 years 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.

#8 @matveb
11 years ago

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

#9 @nacin
11 years 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"?

#10 @nacin
11 years 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.

#11 @nacin
11 years 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.

#12 @nacin
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[28105] was meant for #27738.

#13 @matveb
11 years 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.

@matveb
11 years ago

#14 @matveb
11 years 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.

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


11 years ago

@matveb
11 years ago

#16 @nacin
11 years 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.