Adds query param for version no #4270

aszenz · 2020-06-20T18:29:18Z

This adds support for using a query parameter to select the version no.

Note: If the provided version number in the query string cannot be found then the page is shown as blank since the axios get call returns a 404 and isn't being caught. If this is a concern then a proper 404 message can be shown something like No such version number found.

calebcartwright · 2020-06-20T19:08:18Z

Note: If the version number is not present the page is shown as blank since the axios get call returns a 404 and isn't being caught

Just to clarify, are you saying that if the query parameter is not provided then the page is blank? If so we'd have to fix that, as that behavior would be highly undesirable (and confusing for users!)

calebcartwright · 2020-06-20T19:09:15Z

Or do you mean if someone does something goofy like ?version=fake?

aszenz · 2020-06-20T19:23:04Z

Note: If the version number is not present the page is shown as blank since the axios get call returns a 404 and isn't being caught

Just to clarify, are you saying that if the query parameter is not provided then the page is blank? If so we'd have to fix that, as that behavior would be highly undesirable (and confusing for users!)

No if the query parameter is blank / not provided then it will just select master as it did before.

Or do you mean if someone does something goofy like ?version=fake?

Yes simply a version number that doesn't exist, doesn't have to be goofy, someone could just make a mistake and write a non existent version number and see a blank screen

aszenz · 2020-06-20T19:24:47Z

Note: If the version number is not present the page is shown as blank since the axios get call returns a 404 and isn't being caught

Just to clarify, are you saying that if the query parameter is not provided then the page is blank? If so we'd have to fix that, as that behavior would be highly undesirable (and confusing for users!)

I updated the pr description to clarify my point

calebcartwright · 2020-06-20T19:28:13Z

Thanks for the clarification! Could you update these changes to account for the invalid version scenario?

aszenz · 2020-06-21T05:36:51Z

Thanks for the clarification! Could you update these changes to account for the invalid version scenario?

done, added a message in case the request for the configuration fails

docs/index.html

ayazhafiz

This won't update the query param in the url when the version changes because of a dropdown selection. It may be nice to have this (maybe in another fix).

ayazhafiz · 2020-06-22T16:23:29Z

docs/index.html

+              if (version.startsWith('v')) return version;
+              return `v${version}`;
+            };
+            const versionNumber = null !== versionParam ? parseVersionParam(versionParam) : 'master';


nit: maybe checking for falsyness (!versionParam) is better, in case the param looks like ?version=

hmm, semantically the user has provided a version number, an empty string in this case. so the current way clearly separates version not provided case from provided but invalid case.
If its still necessary will use falsy then

yeah but I think it's fair to assume the user didn't intend to specify a particular version since no valid version will be an empty string

yes, an empty string may not be a valid version but what about ?version=0.0 or ?version=0n.
do we also interpret them as having no intention to specify a version.

IIRC URLSearchParams will return strictly null in the absence of the query param, so I don't want to bikeshed too much on this one.

If anyone's interested in trying to handle the empty string version case then I'd suggest that be done in a follow up

aszenz · 2020-06-22T16:25:34Z

This won't update the query param in the url when the version changes because of a drop down selection. It may be nice to have this (maybe in another fix).

Yes good observation, we would need to bind the selected option with the query parameter.

calebcartwright · 2020-06-23T02:17:50Z

This won't update the query param in the url when the version changes because of a drop down selection. It may be nice to have this (maybe in another fix).

Yes good observation, we would need to bind the selected option with the query parameter.

Good spot! TBH this is another one I could probably live with deferring to a follow up PR, unless you'd be interested in implementing and would prefer to address that within this PR @aszenz?

This adds support for using a query parameter for selecting the version no

Catch request exception in case fetching the configuration from the url fails, this can happen either if non existent version number is passed in or because of server issues.

Covers a few common cases in which the version number can be specified.

aszenz · 2020-06-23T19:46:09Z

Good spot! TBH this is another one I could probably live with deferring to a follow up PR, unless you'd be interested in implementing and would prefer to address that within this PR @aszenz?

Thanks, we have to now link both the search term and version no from the ui to the url, so would prefer to think about it in another pr.
Also rebased this branch on master, to remove conflicts

calebcartwright

Thanks!

* Adds query param for version no This adds support for using a query parameter for selecting the version no * Adds error handling to configuration request Catch request exception in case fetching the configuration from the url fails, this can happen either if non existent version number is passed in or because of server issues. * Makes version selection better Covers a few common cases in which the version number can be specified.

karyon · 2021-10-26T10:08:02Z

backported in #4958

calebcartwright reviewed Jun 21, 2020

View reviewed changes

docs/index.html Outdated Show resolved Hide resolved

ayazhafiz reviewed Jun 21, 2020

View reviewed changes

ayazhafiz reviewed Jun 22, 2020

View reviewed changes

aszenz added 3 commits June 24, 2020 01:10

Adds query param for version no

6173105

This adds support for using a query parameter for selecting the version no

Adds error handling to configuration request

e6ea711

Catch request exception in case fetching the configuration from the url fails, this can happen either if non existent version number is passed in or because of server issues.

Makes version selection better

837caaa

Covers a few common cases in which the version number can be specified.

aszenz force-pushed the patch-1 branch from 8bbc186 to 837caaa Compare June 23, 2020 19:43

calebcartwright approved these changes Jun 23, 2020

View reviewed changes

calebcartwright merged commit 0c337c2 into rust-lang:master Jun 23, 2020

karyon added the 1x-backport:completed label Oct 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds query param for version no #4270

Adds query param for version no #4270

aszenz commented Jun 20, 2020 •

edited

Loading

calebcartwright commented Jun 20, 2020

calebcartwright commented Jun 20, 2020

aszenz commented Jun 20, 2020

aszenz commented Jun 20, 2020

calebcartwright commented Jun 20, 2020

aszenz commented Jun 21, 2020

ayazhafiz left a comment •

edited

Loading

ayazhafiz Jun 22, 2020

aszenz Jun 22, 2020

ayazhafiz Jun 22, 2020

aszenz Jun 22, 2020

calebcartwright Jun 23, 2020

aszenz commented Jun 22, 2020

calebcartwright commented Jun 23, 2020

aszenz commented Jun 23, 2020 •

edited

Loading

calebcartwright left a comment

karyon commented Oct 26, 2021

Adds query param for version no #4270

Adds query param for version no #4270

Conversation

aszenz commented Jun 20, 2020 • edited Loading

calebcartwright commented Jun 20, 2020

calebcartwright commented Jun 20, 2020

aszenz commented Jun 20, 2020

aszenz commented Jun 20, 2020

calebcartwright commented Jun 20, 2020

aszenz commented Jun 21, 2020

ayazhafiz left a comment • edited Loading

Choose a reason for hiding this comment

ayazhafiz Jun 22, 2020

Choose a reason for hiding this comment

aszenz Jun 22, 2020

Choose a reason for hiding this comment

ayazhafiz Jun 22, 2020

Choose a reason for hiding this comment

aszenz Jun 22, 2020

Choose a reason for hiding this comment

calebcartwright Jun 23, 2020

Choose a reason for hiding this comment

aszenz commented Jun 22, 2020

calebcartwright commented Jun 23, 2020

aszenz commented Jun 23, 2020 • edited Loading

calebcartwright left a comment

Choose a reason for hiding this comment

karyon commented Oct 26, 2021

aszenz commented Jun 20, 2020 •

edited

Loading

ayazhafiz left a comment •

edited

Loading

aszenz commented Jun 23, 2020 •

edited

Loading