-
Notifications
You must be signed in to change notification settings - Fork 821
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement client-side routing with Page.js #5243
Implement client-side routing with Page.js #5243
Conversation
Note that the browser support requirement of IE10+ has been discussed and de-facto agreed on through https://groups.google.com/forum/#!topic/silverstripe-dev/ApaaWnlaHpM) |
@silverstripe/core-team We're hoping to get this pull request over the line in the next 48h because we have an opportunity for staff to fix things during an internal hackday here at SilverStripe Ltd on NZ Friday, ideally without too much branching fragmentation. Can you please review in your respective areas of expertise (PHP and/or JS/React)? @flashbackzoo, @scott1702 and myself have largely peer-reviewed each others work here, but would be good to get the thumbs up from at least one other core committer since these pull requests lay the ground work for further React integration. You'll see some |
$priorityA = Config::inst()->get($a, 'url_priority'); | ||
$priorityB = Config::inst()->get($b, 'url_priority'); | ||
|
||
if ($a == $b) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn’t this be if ($priorityA == $priorityB) {
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it should
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are already sorted in AdminRootController::rules() method. I would just move the logic from there to just before the return
in CMSMenu::get_cms_classes
, then you don't have to duplicate this sorting every time it's necessary.
6e3b76c
to
0488e10
Compare
I've added another commit, fixing the issue with window.history.state being null on initial load, and putting some of the sorting for menu items into CMSMain class. |
I think there could be something wrong with the bundles in that commit @tractorcow if you look at the lines added +38,671 LOC in You probably just need to |
- Removes thirdparty dependency History.js - Adds thirdparty dependency Page.js to manage client-side routing - Adds a wrapper around Page.js for SilverStripe specific behaviour - Increased minimum browser requirement to IE10. Native HTML History API routing requires IE10 or newer (necessitated by removal of History.js) - PJAX pannel loading via now uses promises rather than callbacks - Adds getClientConfig method to LeftAndMain which can be used to pass config from to the front-end client
0488e10
to
501b2f1
Compare
Routing required to implement React based CMS sections.
@chillu