guardian / typerighter

Even if you’re the right typer, couldn’t hurt to use Typerighter!
Apache License 2.0
276 stars 12 forks source link

Client-side routing for rule editing #431

Closed jonathonherbert closed 1 year ago

jonathonherbert commented 1 year ago

NB: depends on #430.

What does this change?

Adds client-side routing for rule editing. This lets users link to specific rules, and allows users to navigate between states with back/forward navigation. You know, just internet things.

Screenshot 2023-09-12 at 14 58 09

How to test

Try selecting individual/multiple rules in the rules table. The route will change alongside the selection.

I've added a batch route for the multiple rules path. This is a bit odd – it's just a bit more convenient to model the batch form as a route, but the route itself doesn't have any state, as storing multiple IDs in the URL is a bit weird (it leads to very long URLs and an arbitrary limit on how many we can select, as there's a widely-enforced browser limit of ~2000 chars) Refreshing the page whilst on the batch route will redirect to /. We could refactor to keep batch mode out of the route if that's better.

rhystmills commented 1 year ago

Looking great!

One oddity at the moment is that ticking and then unticking a rule (without making any form changes) no longer closes the rules.

Other than that, all looking good - great to be able to link to rules directly, and I think the 'batch' behaviour is a reasonable compromise.

jonathonherbert commented 1 year ago

Thanks for the review! Good spot, addressed in 1488bbf.

samanthagottlieb commented 1 year ago

This is great! I've tested locally and a couple of things aren't working as I'd expect:

The second point is not something introduced in this PR (previously clicking 'Rules' in the navbar while editing a rule would do nothing), but because the url now resets to '/' and the edit window disappears, I expect it to take me to the Rules 'homepage'.

Not sure if these are things you'd want to change as part of this PR, or if we can instead incrementally build upon this new functionality?

jonathonherbert commented 1 year ago

Thanks for spotting the routing misbehaving, @samanthagottlieb, I'll find a fix!

jonathonherbert commented 1 year ago

The initial routing should be fixed in 8327a36. The state management here is a bit tricky, but hopefully we shouldn't have to worry about it for much longer (rule editing should probably be on a separate page!)

jonathonherbert commented 1 year ago

I've taken a look at the other bug you've spotted, and it's a bit of a pathological case as there are two sources of truth: the route and the component state 😬

This is because there's no good way to store batched ids in the URL. I mentioned this in the PR but didn't think it'd be quite so problematic!

It's patchable as-is, but any solution will take considerable effort and be somewhat brittle, as the code is heavily effect-based and as a result it's often difficult to establish cause and effect.

With that in mind, there are a few options. Context: my feeling is that we probably want rule editing to be a separate page, eventually, to ensure there's space for the rule testing UI.

  1. Find a solution, bearing in mind that this effort will not matter if we do transition to a single page for rules
  2. Leave the bug as-is for now, in the hope that its existence is temporary
  3. Avoid adding routing until we have established a single page for rule editing

Kindof leaning towards 3. What do you think?

samanthagottlieb commented 1 year ago

The initial routing should be fixed in 8327a36. The state management here is a bit tricky, but hopefully we shouldn't have to worry about it for much longer (rule editing should probably be on a separate page!)

Nice!

I've just tested again and noticed that the main rules table doesn't seem to be lining up with what's happening in the url and edit panel. For example, when selecting a couple of rules, then navigating backwards, the multiple rules I initially selected remain selected in the main table, e.g.:

https://github.com/guardian/typerighter/assets/7883129/0ec652d2-f9ae-4b5f-a616-9231f96825ae

Similarly when copying and pasting a rules' url the rule is no longer selected in the main rules table.

With that in mind, there are a few options. Context: my feeling is that we probably want rule editing to be a separate page, eventually, to ensure there's space for the rule testing UI.

  1. Find a solution, bearing in mind that this effort will not matter if we do transition to a single page for rules
  2. Leave the bug as-is for now, in the hope that its existence is temporary
  3. Avoid adding routing until we have established a single page for rule editing

Kindof leaning towards 3. What do you think?

With these few bugs in mind, I'd also lean towards option 3, rather than investing time and effort finding a solution for them. Though if the routing implementation you've added here is likely to be a similar implementation once we've added a single page for rule editing, it's probably fine to live with the bugs for now (i.e. option 2). It sounds like adding a separate editing page is going to be something we implement relatively soon anyway, as we'll need it for rule testing.

As a side point, I'd be keen to hear what you have in mind for a separate rule editing page, as I'm currently working on adding rule creating/editing for dictionary rules.

jonathonherbert commented 1 year ago

Agreed, thanks for taking a look! Closing in favour of a different approach.