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

Allow history viewer screens to be permalinked #1

Open
robbieaverill opened this issue Mar 11, 2018 · 12 comments
Open

Allow history viewer screens to be permalinked #1

robbieaverill opened this issue Mar 11, 2018 · 12 comments

Comments

@robbieaverill
Copy link
Contributor

Originally from: silverstripe/silverstripe-versioned#37

Original A/C from the history viewer - list versions issue:

Each view has a permanent URL (useful e.g. for linking from workflow emails)

Separated out into a separate issue here.

@NightJar
Copy link
Contributor

NightJar commented Jul 1, 2018

Status update since being reassigned to other things:

The version of react-router we're using (in silverstripe/admin) is old.
A bit of a pain, but still probably OK kinda old.

So finding docs is a matter of hitting that tag in the repo and reading the on-github in-repo markdown.
That said, it seems to be a matter of registering the routes for that particular section, complete with children therein. The given example from @unclecheese does not follow the example docs on implementation, rather uses some central registry type approach - presumably to manage all routes within the CMS without re-making routers all over the show. The syntax appears to be as per the react-router docs though.

Similar to the way SS routing works there are variables in-route that allow things like PageID, VersionID etc to be passed down. So then it's mostly just a matter of adding the <Link> components in the appropriate places. My next (current) step was figuring out where exactly those relevant places are - I assumed the full history viewer component (list on down to viewing a specific version), but was in the process of double checking that assumption.

@robbieaverill robbieaverill self-assigned this Jul 1, 2018
@robbieaverill
Copy link
Contributor Author

Observation: react-router implementations that exist already in the SilverStripe core expect a static base URL segment e.g. admin/historyviewer, but our implementation is relatively dynamic in that we provide a HistoryViewerField and a default CMS implementation, so the URLs are always different. This is tricky for when we register the path as something static in ReactRouteRegister

@NightJar
Copy link
Contributor

NightJar commented Jul 2, 2018

How does that play with the base of the router component? I presume there's one high up in the component stack that we don't get to directly play with... but is it possible to layer them (ie. pass control to a new router at the history view level), or perhaps modify (append to) the child routes of an already registered route?

@robbieaverill
Copy link
Contributor Author

We register a route with ReactRouteRegister, giving it the path and any child route patterns.

As I mentioned, the path being static is a problem with this.

@NightJar
Copy link
Contributor

NightJar commented Jul 2, 2018

Yes, but what I'm asking is whether or not it's possible to add child route patterns to an existing registered path?

@robbieaverill
Copy link
Contributor Author

Yeah you can, do you see how that might help?

@NightJar
Copy link
Contributor

NightJar commented Jul 2, 2018

If possible we could find the base path that matches, and add a new child - assuming that all bases must be defined and static, and all routes from there are relative as per https://github.com/ReactTraining/react-router/blob/v2.4.1/docs/API.md#configuration-components

If that's not the case, I'm not really clear on how routes stack themselves down (my understanding is like $url_handlers in SilverStripe PHP) yet aren't considered dynamic...
That was the biggest hole I found in reading the documentation. Perhaps @lukereative might be able to help?

@robbieaverill
Copy link
Contributor Author

robbieaverill commented Jul 2, 2018

If possible we could find the base path that matches

That's the problem, we'd need to define them dynamically.

By default you have these two pre-defined routes (both registered by campaign-admin and asset-admin respectively, which are both totally React driven and have a static base path):

[
  {
    "childRoutes": [
      {"path": ":type/:id/:view"},
      {"path": "set/:id/:view"},
      {"path": "**"}
    ],
    "path": "admin/campaigns"
  },
  {
    "childRoutes": [
      {"path": "show/:folderId/:viewAction/:fileId"},
      {"path": "show/:folderId/:viewAction"},
      {"path": "show/:folderId"},
      {"path": "**"}
    ],
    "path": "admin/assets",
    "indexRoute": {}
  }
]

The default path for history viewer is admin/historyviewer, which doesn't actually match the CMS URL when you're using it so won't help us to implement React based routing.

I'm unsure how to proceed with a dynamic URL, given that when the Javascript boot script runs you don't know what's in the page.

Here are two examples of path values that would be useful for this in the history viewer, but note that they both contain elements that are dynamic and neither necessarily imply that they're a history viewer based view in the CMS:

  • /admin/pages/history/show/6
  • /admin/pages/edit/EditForm/6/field/ElementalArea/item/1/edit

Moving to blocked for now. Would be good to get @unclecheese's or @flamerohr's input on how we might achieve this.

@robbieaverill robbieaverill removed their assignment Jul 2, 2018
@NightJar
Copy link
Contributor

NightJar commented Jul 2, 2018

To that end it's probably also worth adding that presently the route
/admin/pages/history/show/6 could be our react history viewer
while
/admin/pages/history/show/3 may not be
by manner of a page having to have an extension applied to enable the history viewer

if ($page && $this->isEnabled($page)) {

The second route ties into #22 which I've found to be caused by the history being served from a (legacy style) content editing tab - and as such may all change in the way it's handled.

I also need to read the router docs more to see if child routes are also able to access all parsed variables from their parents e.g. item/:id/history where the child route may history in order to allow edit to be another route tied to another component.

@chillu
Copy link
Member

chillu commented Jan 28, 2019

Hrm not super happy that this has been ignored in the original ACs, and then left rotting for 7 months :/

@ScopeyNZ
Copy link
Contributor

@robbieaverill considering recent efforts with the CKAN module and routing has a potential solution perhaps a little clearer? Sorry I haven't got much context around this but it seems somewhat similar.

As an example; permalinking version 8 of page 6 (eg. /admin/pages/6/history/8) should be achievable by having a catch-all of admin/pages/6/history and then using that as a base URL with the React router?

@robbieaverill
Copy link
Contributor Author

Yep, should be easy enough to plug in. Also consider use in GridFields. We would need to do the same as CKAN where we disable some sub URLs so the react router can have them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants