-
Notifications
You must be signed in to change notification settings - Fork 9
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
Sundry fixes #865
Sundry fixes #865
Conversation
@@ -21,6 +26,17 @@ import {ArticlesNavManualList} from '~/components/ArticlesNav/ArticleNavManualLi | |||
export const LINK_WITHOUT_DETAILS_CLS = 'link-without-details' | |||
|
|||
export const loader = async ({request, params}: LoaderFunctionArgs) => { | |||
const {data: redirects} = await loadRedirects(request) | |||
|
|||
// Check if we need to redirect to a new path |
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.
/questions/<page id>
redirects weren't working, because they were being caught by this route and rendered as normal questions
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.
https://aisafety.info/questions/8TJV/How-can-I-help
now renders as "deleted", but the redirection is not working (see https://coda.io/d/AI-Safety-Info_dfau7sl2hmG/Redirects_suCc9XjM#Redirects_tuDQLOm3)
This is a local improvement over the previous situation, but I'd still rather redirections take priority over deleted articles
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.
It seems like all redirections now fail, https://aisafety.info/help won't resolve either
@@ -163,6 +179,8 @@ export default function RenderArticle() { | |||
return ( | |||
<Error error={{statusText: 'Could not fetch question', status: 'emptyArticle'}} /> | |||
) | |||
} else if (resolvedQuestion.data.status === QuestionStatus.TO_DELETE) { |
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.
any other states that should display errors?
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.
Duplicate
They are used essentially in the same way, they can both have the same error msg.
({tags, status}) => | ||
tags?.includes(INTRODUCTORY) || | ||
tags?.includes(ADVANCED) || | ||
status === QuestionStatus.SUBSECTION |
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 could mess up things - if a subsection isn't tagged as introductory or advanced then it may show up somewhere it shouldn't. This seemed like the simplest way to do it, though
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.
Subsections are actually never tagged as either introductory or advanced, only sections are tagged as such. Still, I think the code makes sense. Did you mean that weird things will happen if a subsection is tagged as introductory or advanced?
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 don't know if they will happen, but I'm not certain that they won't...
why are the tests failing... :/ |
/question/<pageid>
redirects