-
Notifications
You must be signed in to change notification settings - Fork 149
feat(artist): redirect old artist subpage routes to unified page anchors #16457
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
base: main
Are you sure you want to change the base?
Conversation
This will allow us to use just the query munging logic in the new redirect we are about to write next.
#5470 Bundle Size — 9.51MiB (-0.01%).15edf90(current) vs 287f2a8 main#5464(baseline) Warning Bundle contains 34 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch anandaroop/DIAM-218-redirects-v2 Project dashboard Generated by RelativeCI Documentation Report issue |
91bd3d0 to
efcafb5
Compare
efcafb5 to
15edf90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performing the redirects here with the RedirectException pattern.
| render: ({ match }) => { | ||
| const redirectUrl = `/artist/${match.params.artistID}#JUMP--artistAboutTop` | ||
| throw new RedirectException(redirectUrl, 301) | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and above I use the anchor that already exists in the markup, then use the the effect to scroll to the content, and remove the hash from the URL.
I felt the urge to remove it because it seems to have kind of an "internal" (JUMP--…) feel to it.
But should we leave the hash? I guess that would make auction/about URLs be more copy-pasteable 🤷🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this file so that I could use just the query namespacing logic, separated out from the redirecting logic.
Tbh I'm not quite sure what all's going on here, e.g. with the casing stuff, but I've preserved the existing behavior.
| // When the page is accessed via a URL that contains a hash to a jump link | ||
| // anchor -- as in the case of the 301 redirects we've set up -- | ||
| // we similarly scroll to the appropriate section and clean up the URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is similar but not exactly the same as the other scrollToSection so it seemed reasonable to duplicate it here.
Caution
Unmergeable until unified artist page is updated and rolled out to 100% of users. This PR may require some rebasing once that happens.
The type of this PR is: Feat
This PR solves DIAM-218
Description
Sets up the permanent redirects we want in light of the change to the unified artist page.
301 Redirect
In order to preserve link juice for these now obsolete routes we want to 301 redirect them to their unified artist page counterparts:
Namespaced auction params
We preserve the pre-existing logic to munge auction-related params into a namespace…
…in order to play nice with the unified page logic.
Scroll to content
And we preserve the behavior of scrolling to the appropriate section once it is ready (and clean up the current URL accordingly).
Result
The net result is a sequence like the following
User follows an old link:
/artist/banksy/auction-results?sort=ESTIMATE_AND_DATE_DESCServer detects the route & params and then 301 redirects:
/artist/banksy?auction%5Bsort%5D=ESTIMATE_AND_DATE_DESC#JUMP--marketSignalsTopClient detects the hash and waits-then-scrolls to the appropriate section, then cleans up the URL
/artist/banksy?auction%5Bsort%5D=ESTIMATE_AND_DATE_DESC