Appearance Themes: Backbone.Router should use pushState with ?query type urls

[matveb]

Make the Backbone routes pushSTate capable with ?theme=themename type urls instead of hashes. Same applies to search queries.

[adamsilverstein]

Enabling pushState: true removes the hash marks from URLS so we get pretty URLS like themes.php/theme/twentyeleven. I think the only reason we were shooting for routes via query parameter like themes.php?theme=twentyeleven is to match the layout of the revisions URLS.

According to the Backbone forums ( see ​here, ​here and ​here ) query args are now completely ignored by Backbone.Router, and the authors are of the opinion that client side code shouldn't use query parameters at all.

I tried matching the setup we used in revisions to map the route, but when using '?' my route is never fired. There are workarounds, and ​this plugin adds query args. I think this deserves further discussion: do we really need query arg route mapping? I think revisions kept the ?revision=ID mapping originally to match the previous system's URLS - I don't think we have that need here and I'm inclined to use regular 'path mapping'.

[adamsilverstein]

add pushState: true

[matveb]

I had it working with pushState in the form of themes.php/theme/twentyeleven but it felt a bit awkward so went with hashes instead.

[carldanley]

Let me know if you need something put together for this; would love to help!

[adamsilverstein]

Replying to carldanley:

Let me know if you need something put together for this; would love to help!

Your expertise & input appreciated! the original idea was to use query string urls like we currently use in revisions. I wasn't able to get it working without some kludgy code. The url would look like:

wp-admin/themes.php?theme=[theme_name]

I believe the impetus was to match revisions and possibly provide a no-js fallback (since the theme name is passed to PHP this way). However, Backbone seems to be moving away from allowing/supporting query string routing. if we just enable pushState, we get URLS like this:

wp-admin/themes.php/theme/[theme_name]

which is OK, perhaps a bit prettier would be something like

wp-admin/themes/[theme_name] which should require adding some logic to wp-admin/index.php

you will also note a search route in the code, so far we've only talked about the theme route.

[ocean90]

Currently there is an issue with the browser history and closing the modal.

• Go to wp-admin/themes.php
• Open info modal of Twenty Eleven
• Navigate to the next theme, Twenty Fourteen for me
• Click browsers back button
• You should see Twenty Eleven again
• Try to close the modal
• Modal isn't closed, it shows Twenty Fourteen again
• Try to close the modal again
• Now it's closed

So if you are using browsers back button x-times you have to click the modal close icon x-times too.

[nacin]

We need to figure this out. I see two problems:

• The bug mentioned by ocean90
• The lack of query string URLs

I actually find the lack of query string URLs to be a pretty major issue. Not as much for ?theme= but for ?s=, which is broken. :-( Otherwise there is no good way to "link" someone to a particular theme.

[carldanley]

I didn't have a ton of time but hopefully this will help somehow. I put together a function for you that parses the GET params of a URL.

​https://gist.github.com/carldanley/e3e1f609091f67356f59

You can play around with this function here: ​http://jsfiddle.net/zwb8E/ .

• If the URL contains multiple ?'s, the last ?'s parameters are used.
• If the URL contains an empty GET param, it will have a value of undefined.

My idea is that this can be wrapped into something, somewhere to help you support this. Let me know if you have any questions.

[ocean90]

Related router part from revisions: trunk/src/wp-admin/js/revisions.js@26162#L1051​

[adamsilverstein]

Replying to ocean90:

Related router part from revisions: trunk/src/wp-admin/js/revisions.js@26162#L1051​

when i tried this approach with the current code it failed to call the routes at all. i will take another look!

[adamsilverstein]

correct duplicate overlay back button issue

[adamsilverstein]

Replying to ocean90:

Currently there is an issue with the browser history and closing the modal.

• Go to wp-admin/themes.php
• Open info modal of Twenty Eleven
• Navigate to the next theme, Twenty Fourteen for me
• Click browsers back button
• You should see Twenty Eleven again
• Try to close the modal
• Modal isn't closed, it shows Twenty Fourteen again
• Try to close the modal again
• Now it's closed

So if you are using browsers back button x-times you have to click the modal close icon x-times too.

25963.2.diff​ corrects issue with back button - multiple overlays were being created, this change prevents that. working on pushState next. please review.

[adamsilverstein]

adds ?theme=name routing; also pushState: true

[adamsilverstein]

In 25963.3.diff​:

• added ?theme=name routing; also pushState: true
• search routing next!

[carldanley]

Looks good. L708 needs indent though. Also, maybe some function commenting to keep things organized. Good stuff adamsilverstein :)

[adamsilverstein]

cleaner fix for duplicate overlay back button issue

[adamsilverstein]

25963.4..diff​​ is a cleaner fix for the back button issue mentioned ​above by ocean90.

[adamsilverstein]

routing following revisions.js pattern

[adamsilverstein]

in 25963.5.diff​

• use a routing methodology similar to revisions - php checks for initially passed variables and JS sets state appropriately
• back button no longer works (although reloading or bookmarking works)
• testing revisions again in trunk and back to 3.6.1 I'm seeing that the back button also doesn't work there so behavior ok?

[adamsilverstein]

25963.6.diff​:

• removes unused routes
• don't clutter history - navigate with replace: true

[adamsilverstein]

simpler, remove unused routes, don't clutter history

[adamsilverstein]

remove unused self = this

[kovshenin]

25963.8.diff​ deals with the keyboard navigation lag.

The issue occurs inside themes.view.Details.navigation() which gets fired during .render() meaning a new keyup event is attached to body every time the details view is rendered. After a while this becomes very laggy. The patch moves the event binding to themes.view.Themes.initialize() instead, and calls the overlay methods directly, which then trigger the appropriate events.

[adamsilverstein]

combines 7 & 8, fixes missing event in collapse call

[adamsilverstein]

looks good. the only issue i found was firefox throwing an undefined console error on the collapse call.

25963.9.diff​​ corrects that by explicitly passing the event to collapse and combines the changes from 25963.7.diff​ and 25963.8.diff​

[nacin]

How's it looking? This about ready? :-) What still needs to be done?

[adamsilverstein]

remove search debounce as per IRC discussion

[adamsilverstein]

works well in my testing. would like to try more testing in RTL/various browsers

[nacin]

This works beautifully in my testing. 25963.11.diff​ updates it to trunk. It also removes extraRoutes.

[ryan]

Make the Backbone routes pushSTate capable with ?theme=themename type urls instead of hashes. Same applies to search queries.
Props adamsilverstein, nacin

[26767]

[nacin]

In 26778:

Fix JSHint errors and remove unreachable code. props adamsilverstein. see #25963.

[matveb]

I have been working on a possible solution that will allow us to use routing instead of passing the url params via localize script. We can parse the url parameters from Backbone.history and then trigger proper route:theme events which we hook to the same way we were doing before.

I have it working but things got out of sync with the latest commit here. This is what I propose, though, what do you think?

​https://cloudup.com/cPHH02p4W9s

[adamsilverstein]

restore real JS routing

[adamsilverstein]

Replying to matveb:

I have been working on a possible solution that will allow us to use routing instead of passing the url params via localize script. We can parse the url parameters from Backbone.history and then trigger proper route:theme events which we hook to the same way we were doing before.

I have it working but things got out of sync with the latest commit here. This is what I propose, though, what do you think?

​https://cloudup.com/cPHH02p4W9s

(The RegEx can probably be tweaked to be more robust.)

Excellent - I really prefer this all JavaScript approach to the routing - it seems to work well and is pretty straightforward (other than the extracting the args from Backbone.history.location.search). It deserves more testing, hopefully this patch helps with that.

25963.12.diff​ introduces your approach against trunk and removes the wp_localize_script pass of theme and search vars.

we should add a ?s=themename url for backward compatibility with existing links (or how about just keeping ?s=themename as the search string?)

[matveb]

Great! I started a more general purpose ticket for solving this at a wp-backbone level instead, so also revisions (and anything new) benefits from it. See #26543.