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

feature: preview links in CMS #233

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

feature: preview links in CMS #233

wants to merge 4 commits into from

Conversation

jbmoelker
Copy link
Member

@jbmoelker jbmoelker commented Dec 21, 2024

To do

  • Agree on one rerouting/redirecting solution, remove the other.
  • Get preview secret to be used automatically. If click on the preview links in the sidebar you still need to enter the secret manually (I don't get why). While if you copy the link and paste it in a new browser tab it works as expected.

Changes

  • Adds preview links to the CMS so an editor can easily preview any type of page record (home, generic page, not found page)

This change contains 2 solutions:

  • an /api/reroute/page endpoint to find and redirect to the correct page url
  • a check in pages/[locale]/[...path]/index.astro that redirects to the canonical url if it doesn't match the current url

Associated issue

Part of #10

How to test

  1. Open the CMS and go to the preview-links environment (note: it's moved from personal to De Voorhoede organisation account, so we could add more sandbox environments)
  2. Open the sidebar in the home / page / not found page model
  3. Try the preview links
  4. Verify that they all work as expected

Checklist

  • I have performed a self-review of my own code
  • I have made sure that my PR is easy to review (not too big, includes comments)
  • I have made updated relevant documentation files (in project README, docs/, etc)
  • I have added a decision log entry if the change affects the architecture or changes a significant technology
  • I have notified a reviewer

This change contains 2 solutions:
- an `/api/reroute/page` endpoint to find and redirect to the correct page url
- a check in `pages/[locale]/[...path]/index.astro` that redirects to the canonical url if it doesn't match the current url
Copy link

cloudflare-workers-and-pages bot commented Dec 21, 2024

Deploying head-start with  Cloudflare Pages  Cloudflare Pages

Latest commit: a2391ec
Status: ✅  Deploy successful!
Preview URL: https://bba16df6.head-start.pages.dev
Branch Preview URL: https://feat-preview-links.head-start.pages.dev

View logs

});
};

export const GET: APIRoute = async ({ request }) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a generic alternate approach that I used in the nododos-website. But I think the canonical url check in [locale]/[...path]/index.astro is easier and probably the way to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm. I guess the canonical check works on localhost and preview as the generic page route is then always handled dynamically on the server. However the production environment has prerendered pages and will throw a 404 I suppose. So I guess we do need this reroute API handler.

In that case the config of the Preview field of the generic page will need to use that one instead.

const { locale } = Astro.params as { locale: SiteLocale };
const localeFromPath = Astro.params.locale as SiteLocale;
const localeFromQuery = Astro.url.searchParams.get('locale') as SiteLocale;
const locale = localeFromQuery || localeFromPath;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now needed to have the Astro.rewrite('/404/') work in [locale]/[...path]/index.astro. It's the tradeoff for no longer needing to cast the response from the page query. Worth it? Or rather have that one fail as it did before?

const breadcrumbs = [...getParentPages(page), page].map((page) =>
const { page } = await datocmsRequest<PageQuery>({ query, variables });

if (!page) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This check ensures this type casting is no longer needed:

as {
  page: NonNullable<PageQuery['page']>; // Only NonNullable when statically generated. Handle as a 404 when this is a server route!
};

But it makes the 404 rewrite and locale setting in that file more complex. Worth it?


#### Model Deployments Links plugin

The [Model Deployment Links plugin](https://www.datocms.com/marketplace/plugins/i/datocms-plugin-model-deployment-links) requires an API key:
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this could also be achieved within a migration script using https://www.datocms.com/docs/content-management-api/resources/access-token/create . However it's difficult to reliably get the Editor role ID. Since this is a one time config step, I'm happy to have it as a manual step, rather than scripting the generation of access token.

@jbmoelker jbmoelker added the help wanted Extra attention is needed label Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed 🚏 routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant