code-lts / doctum

A php API documentation generator, fork of Sami
https://doctum.long-term.support/
MIT License
300 stars 32 forks source link

Attempt to reload current page in newly-selected version #60

Closed andrewandante closed 7 months ago

andrewandante commented 1 year ago

Inspired by https://github.com/silverstripe/api.silverstripe.org/issues/106

Most of the time when I toggle the version switcher, I'm hoping to see the page I am currently on, but in a different version.

This PR attempts to do that, rather then defaulting to the homepage of the newly-selected version.

williamdes commented 1 year ago

That's a good idea ! What does it do when it does not exist on the target version ?

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 63.92%. Comparing base (2ca5517) to head (0447854). Report is 23 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #60 +/- ## ============================================ - Coverage 64.31% 63.92% -0.39% - Complexity 1241 1254 +13 ============================================ Files 53 53 Lines 3584 3615 +31 ============================================ + Hits 2305 2311 +6 - Misses 1279 1304 +25 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

andrewandante commented 1 year ago

That's a good idea ! What does it do when it does not exist on the target version ?

It just 404s. Which is not-great, I think, but can be useful as "oh this class doesn't exist in version 4" or whatever, so I'm somewhat ok with it as a concept.

williamdes commented 1 year ago

That's a good idea ! What does it do when it does not exist on the target version ?

It just 404s. Which is not-great, I think, but can be useful as "oh this class doesn't exist in version 4" or whatever, so I'm somewhat ok with it as a concept.

Okay, well maybe we can find some middle ground with this Maybe asking the search bar to load the page that we where on before Like some return_to argument

andrewandante commented 1 year ago

We could sort of naively check for a 200-399 response before re-routing? Something like (pseudocode):

var headReq= new XMLHttpRequest();
headReq.open('HEAD', url, false);
headReq.send();
if (headReq.status < 200 || headReq.status > 399)
  return newVersionIndex;
else
  return url;

Or something like that. Essentially, the old behaviour unless the page exists

williamdes commented 1 year ago

I like this idea ! Not all servers support HEAD, but we can agree to say that they should and ignore this corner case. Or just try using GET if the first one fails

andrewandante commented 1 year ago

agree to say that they should

Yeah I think that's the approach - if it's unsupported it should return a 405 (I think) and the existing behaviour will resurface anyway. I've pushed an update to support this HEAD request

williamdes commented 7 months ago

Thank you for your work and patience, this is a great feature !

andrewandante commented 7 months ago

Thanks @williamdes - though I've just had a cursory glance at the code I wrote and think I might have the conditionals around the wrong way?