Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#25963 closed enhancement (fixed)

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

Reported by: matveb's profile matveb Owned by:
Milestone: 3.8 Priority: high
Severity: major Version: 3.8
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

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

Attachments (14)

25963.diff (711 bytes) - added by adamsilverstein 11 years ago.
add pushState: true
25963.2.diff (796 bytes) - added by adamsilverstein 11 years ago.
correct duplicate overlay back button issue
25963.3.diff (4.5 KB) - added by adamsilverstein 11 years ago.
adds ?theme=name routing; also pushState: true
25963.4.diff (902 bytes) - added by adamsilverstein 11 years ago.
cleaner fix for duplicate overlay back button issue
25963.5.diff (6.3 KB) - added by adamsilverstein 11 years ago.
routing following revisions.js pattern
25963.6.diff (5.5 KB) - added by adamsilverstein 11 years ago.
simpler, remove unused routes, don't clutter history
25963.7.diff (5.2 KB) - added by adamsilverstein 11 years ago.
remove unused self = this
25963.8.diff (1.7 KB) - added by kovshenin 11 years ago.
25963.9.diff (6.8 KB) - added by adamsilverstein 11 years ago.
combines 7 & 8, fixes missing event in collapse call
25963.10.diff (6.8 KB) - added by adamsilverstein 11 years ago.
remove search debounce as per IRC discussion
25963.11.diff (7.4 KB) - added by nacin 11 years ago.
25963.regex-route-concept.diff (7.7 KB) - added by matveb 11 years ago.
25963.12.diff (3.8 KB) - added by adamsilverstein 11 years ago.
restore real JS routing
26543.13.patch (4.2 KB) - added by gcorne 10 years ago.

Download all attachments as: .zip

Change History (44)

#1 @adamsilverstein
11 years ago

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
11 years ago

add pushState: true

#2 @matveb
11 years ago

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.

#3 @SergeyBiryukov
11 years ago

  • Milestone changed from Awaiting Review to 3.8

#4 @carldanley
11 years ago

  • Cc carldanley@… added

#5 follow-up: @carldanley
11 years ago

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

#6 in reply to: ↑ 5 @adamsilverstein
11 years ago

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.

#7 follow-up: @ocean90
11 years ago

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.

#8 @rmccue
11 years ago

  • Severity changed from normal to minor

#9 @nacin
11 years ago

  • Priority changed from normal to high
  • Severity changed from minor to major

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.

#10 @carldanley
11 years ago

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.

#11 follow-up: @ocean90
11 years ago

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

#12 in reply to: ↑ 11 @adamsilverstein
11 years ago

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
11 years ago

correct duplicate overlay back button issue

#13 in reply to: ↑ 7 @adamsilverstein
11 years ago

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
11 years ago

adds ?theme=name routing; also pushState: true

#14 @adamsilverstein
11 years ago

In 25963.3.diff:

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

#15 @carldanley
11 years ago

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

@adamsilverstein
11 years ago

cleaner fix for duplicate overlay back button issue

#16 @adamsilverstein
11 years ago

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

@adamsilverstein
11 years ago

routing following revisions.js pattern

#17 @adamsilverstein
11 years ago

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?

#18 @adamsilverstein
11 years ago

25963.6.diff:

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

@adamsilverstein
11 years ago

simpler, remove unused routes, don't clutter history

@adamsilverstein
11 years ago

remove unused self = this

@kovshenin
11 years ago

#19 @kovshenin
11 years ago

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
11 years ago

combines 7 & 8, fixes missing event in collapse call

#20 @adamsilverstein
11 years ago

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​

#21 @nacin
11 years ago

  • Keywords has-patch added

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

@adamsilverstein
11 years ago

remove search debounce as per IRC discussion

#22 @adamsilverstein
11 years ago

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

@nacin
11 years ago

#23 @nacin
11 years ago

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

#24 @nacin
11 years ago

  • Keywords commit added

#25 @ryan
11 years ago

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

[26767]

#26 @ryan
11 years ago

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

#27 @nacin
11 years ago

In 26778:

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

#28 follow-up: @matveb
11 years ago

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

Version 0, edited 11 years ago by matveb (next)

@adamsilverstein
11 years ago

restore real JS routing

#29 in reply to: ↑ 28 @adamsilverstein
11 years ago

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?)

#30 @matveb
11 years ago

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.

@gcorne
10 years ago

Note: See TracTickets for help on using tickets.