Skip to content
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

Add a way of marking articles as deprecated #3912

Merged
merged 6 commits into from
Sep 5, 2024
Merged

Conversation

rakyi
Copy link
Contributor

@rakyi rakyi commented Aug 30, 2024

No description provided.

@rakyi rakyi requested a review from ikesau August 30, 2024 10:15
@owidbot
Copy link
Contributor

owidbot commented Aug 30, 2024

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-deprecated-articles

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2024-08-30 10:22:40 UTC
Execution time: 1.18 seconds

@rakyi rakyi force-pushed the deprecated-articles branch from c3bb96a to 4409b25 Compare August 30, 2024 13:27
@rakyi rakyi linked an issue Aug 30, 2024 that may be closed by this pull request
@rakyi rakyi requested review from danyx23 and ikesau and removed request for ikesau and danyx23 August 30, 2024 15:29
@rakyi rakyi marked this pull request as ready for review August 30, 2024 15:36
@rakyi rakyi marked this pull request as draft August 30, 2024 15:47
@rakyi rakyi marked this pull request as ready for review September 2, 2024 12:22
@rakyi
Copy link
Contributor Author

rakyi commented Sep 2, 2024

This is ready for review, but there is a chance the design will change slightly.

Copy link
Member

@ikesau ikesau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Just a few code nitpicks really.

Do you know if we still want to index deprecated articles and show them in the sitemap?

@rakyi rakyi requested a review from ikesau September 4, 2024 08:16
@ikesau
Copy link
Member

ikesau commented Sep 4, 2024

Hey @rakyi sorry I went ahead and yak-shaved on your work without your blessing, but I realized earlier today in the cursor demo that my suggestion of using 0 | 1 still felt a bit code-smelly to me, so I investigated turning it into a boolean instead which MySQL can't do, and well, one thing led to another 😓

The only truly critical fix I made was in GdocFactory where I noticed a leading slash was there where it shouldn't be, which is what made me realize we should be using constants for these 2 filenames.

If you'd prefer, you can revert my changes (though you'll need to fix the leading slash again) and I'll add the constants in a separate PR.

Also, do we want to exclude archived posts from the sitemap and search index? 🙂

@rakyi
Copy link
Contributor Author

rakyi commented Sep 5, 2024

Thank you for the changes!

Also, do we want to exclude archived posts from the sitemap and search index? 🙂

I forgot to reply to this before. We shouldn't exclude these posts from search and sitemap.

@ikesau
Copy link
Member

ikesau commented Sep 5, 2024

Once you merge this, don't forget to add an example to the article documentation gdoc 🙂

You'll probably have to write it in the body and escape it so that ts-pattern doesn't complain - otherwise the docs themselves will have the deprecation notice which we probably don't want.

@rakyi
Copy link
Contributor Author

rakyi commented Sep 5, 2024

Yeah, I was meaning to do that. Thank you for the tip!

@rakyi rakyi merged commit bd00cab into master Sep 5, 2024
21 checks passed
@rakyi rakyi deleted the deprecated-articles branch September 5, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to mark articles as deprecated
3 participants