Skip to content
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

API Implement react-router in frontend example app #136

Merged

Conversation

robbieaverill
Copy link
Contributor

  • Rename example app entrypoint to CKANExampleApp, with entwine binding
  • Fix bug with duplicate containers causing duplicate React apps to be rendered
  • Add initial data to CKAN client config
  • Disable ability for CKAN pages to have children (interferes with frontend routing)
  • Add initial template for frontend detail view, and mock links for routing examples

Resolves #79

* Rename example app entrypoint to CKANExampleApp, with entwine binding
* Fix bug with duplicate containers causing duplicate React apps to be rendered
* Add initial data to CKAN client config
* Disable ability for CKAN pages to have children (interferes with frontend routing)
* Add initial template for frontend detail view, and mock links for routing examples
src/Model/Resource.php Outdated Show resolved Hide resolved
@ScopeyNZ
Copy link
Member

Okay I left one comment about coupling to pages. I'll leave an approval of the PR though if not coupling is in the "too hard basket". We can always come back and fix this later.

I've investigated the /detail/:id idea. It works with minimal changes. The only issue is that you have to blacklist /detail. @NightJar indicated this might be automatic of sorts but in my testing I was able to create subpages so that this route did not apply:

private static $url_handlers = [
    // Route all requests for this page, including sub-URLs, to the index action.
    // The frontend components should take care of handling sub-URL routing from here.
    'detail/$Item' => 'index',
];

So the solution in the end involves adding an extension to SiteTree through the DataObject::validate extension and then making sure if it's a child of a CKANRegistryPage that the URL segment is not bad. I didn't have time to do this but I can raise a PR over the weekend if the issue is till outstanding.

@robbieaverill
Copy link
Contributor Author

robbieaverill commented Jan 18, 2019

I guess that’s no different to existing modules that have controller actions eg blog. I wouldn't worry too much about it for now.

I've opted to use "view" instead of "detail", sound OK?

Will update the PR to remove the coupling to Page and have it accept a DataObject instead

@robbieaverill
Copy link
Contributor Author

Both points addressed and pushed

@ScopeyNZ ScopeyNZ merged commit 5851ba9 into silverstripe:master Jan 18, 2019
@ScopeyNZ ScopeyNZ deleted the pulls/1.0/react-router branch January 18, 2019 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants