WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 12 months ago

Last modified 12 months ago

#26543 closed enhancement (fixed)

Backbone Routing doesn't support query arguments as parseable route components. WordPress uses them extensively, so it'd be worthwhile to support them in wp-backbone.js for ease of use.

Reported by: matveb Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.8
Component: General Keywords: has-patch needs-testing
Focuses: javascript Cc:

Description

We currently have a few pieces of wp-admin relying on Backbone. One issue is that we cannot use pushState navigation with GET params natively from Backbone's routing system, and WordPress uses them extensively.

The current approaches at solving this complicate the code unnecessarily, and mostly miss the chance of using events to handle location changes on the front end. (Basically, the routing system becomes blind to location changes.)

We should instead extend Backbone.History to support basic query type fragments as routes so we can natively use
route:events instead of the way we are handling it now — parsing the URL with PHP and sending the values to localize_script and triggering stuff blindly.

Instead of hacking solutions every time we do a Backbone implementation, let's provide some helper code from
wp-backbone.js.

Revisions and Themes would benefit greatly from it and the simplified code. Any subsequent Backbone implementation on the admin would also be able to easily register ?param=value routes.

Concept: https://cloudup.com/c0ZdVFJ1Sky


Attachments (4)

26543-wp-backbone.diff (1.8 KB) - added by matveb 15 months ago.
26543-theme-js.diff (2.0 KB) - added by matveb 15 months ago.
26543.diff (4.5 KB) - added by adamsilverstein 15 months ago.
combine patches & remove php passed vars
26543-ie8-routing-fix.diff (4.8 KB) - added by ehg 13 months ago.
modify combined patch to fix IE8/non-pushState routing bug (L42)

Download all attachments as: .zip

Change History (20)

comment:1 @ocean9015 months ago

  • Cc ocean90 added

comment:2 @adamsilverstein15 months ago

  • Cc adamsilverstein@… added

@matveb15 months ago

@matveb15 months ago

comment:3 @matveb15 months ago

With 26543-wp-backbone.diff​ you can now start Backbone.history with an optional queryUri param that triggers the query var support:

Backbone.history.start({ pushState: true, queryUri: true });

If you don't pass it you should get vanilla Backbone routing.

comment:4 @matveb15 months ago

26543-theme-js restores native routes like the following:

routes: {
	'themes.php?theme=:slug': 'theme',
	'themes.php?search=:query': 'search',
	'themes.php?s=:query': 'search'
}

comment:5 @adamsilverstein15 months ago

works well in my testing, nice work! like that we don't need to pass the vars from php any more.

@adamsilverstein15 months ago

combine patches & remove php passed vars

comment:6 @adamsilverstein15 months ago

26543.diff​ combines patches & removes php passed vars

comment:7 @ehg15 months ago

  • Cc ehg added

comment:8 @rmccue15 months ago

  • Keywords has-patch added

+1000.

comment:9 @matveb15 months ago

  • Keywords needs-testing added

comment:10 @jeremyfelt13 months ago

  • Focuses javascript added

@ehg13 months ago

modify combined patch to fix IE8/non-pushState routing bug (L42)

comment:11 @matveb13 months ago

Thanks for the patch, ehg.

From Backbone 1.1.1 release notes: Added an execute hook to the Router, which allows you to hook in and custom-parse route arguments, like query strings, for example. Hat-tip mattwiebe

comment:12 @ircbot13 months ago

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

comment:13 @gcorne13 months ago

If we upgrade Backbone to 1.1.2, the patching of Backbone.History is no longer necessary. See #27182

For future reference, if patching is needed it would be better to extend Backbone.History to create a new wp.Backbone.History constructor. And, then in places where we need our version use Backbone.history = new wp.Backbone.History(), which would negate the need for idioms like originalFragment: Backbone.History.prototype.getFragment.

I also noticed that some of the navigate() calls should probably not use replace: true so that entries are added to the browser's history so that the back button still works. The changes to how routing is handled in THX probably should be a separate ticket.

comment:14 @matveb13 months ago

Thanks for the look. The original separate THX ticket that led to this one: #25963. If anyone feels like updating the latest patch to make use of the new Router hooks and avoid modifying Backbone.History to parse the queries, be my guest. Otherwise I'll try to get to it sometime next week. It's probably worth starting a new one.

(Also related: #26565.)

comment:15 @gcorne12 months ago

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

Opened #27198 to cover the routing improvements and added a patch that I think is ready to go. Closing this ticket since r27233 accomplishes the main goal.

comment:16 @SergeyBiryukov12 months ago

  • Milestone changed from Awaiting Review to 3.9
Note: See TracTickets for help on using tickets.