#25963 closed enhancement (fixed)
Appearance Themes: Backbone.Router should use pushState with ?query type urls
Reported by: | 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)
Change History (44)
#2
@
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.
#5
follow-up:
↓ 6
@
11 years ago
Let me know if you need something put together for this; would love to help!
#6
in reply to:
↑ 5
@
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:
↓ 13
@
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.
#9
@
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
@
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:
↓ 12
@
11 years ago
Related router part from revisions: trunk/src/wp-admin/js/revisions.js@26162#L1051
#12
in reply to:
↑ 11
@
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!
#13
in reply to:
↑ 7
@
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.
#14
@
11 years ago
In 25963.3.diff:
- added ?theme=name routing; also pushState: true
- search routing next!
#15
@
11 years ago
Looks good. L708 needs indent though. Also, maybe some function commenting to keep things organized. Good stuff adamsilverstein :)
#16
@
11 years ago
25963.4..diff is a cleaner fix for the back button issue mentioned above by ocean90.
#17
@
11 years ago
- 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?
#19
@
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.
#20
@
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
@
11 years ago
- Keywords has-patch added
How's it looking? This about ready? :-) What still needs to be done?
#23
@
11 years ago
This works beautifully in my testing. 25963.11.diff updates it to trunk. It also removes extraRoutes.
#25
@
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
#28
follow-up:
↓ 29
@
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
(The RegEx can probably be tweaked to be more robust.)
#29
in reply to:
↑ 28
@
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 properroute: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?)
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 likethemes.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'.