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

fix: adapt base url to netlify #784

Merged
merged 4 commits into from
Nov 28, 2023
Merged

fix: adapt base url to netlify #784

merged 4 commits into from
Nov 28, 2023

Conversation

milljoniaer
Copy link
Contributor

@milljoniaer milljoniaer commented Nov 28, 2023

What was done?

The Base URL was previously adapted to gatsby cloud pr deployments. This PR changes that to the netlify pattern.

How to verify?

Verify that on the preview of this PR, the base URL of the meta images is the same as the preview URL:
image

Copy link

netlify bot commented Nov 28, 2023

Deploy Preview for satellytes ready!

Name Link
🔨 Latest commit 714b21a
🔍 Latest deploy log https://app.netlify.com/sites/satellytes/deploys/6565d0e1187b4700089db8e3
😎 Deploy Preview https://deploy-preview-784--satellytes.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 88 (🔴 down 1 from production)
Accessibility: 94 (no change from production)
Best Practices: 100 (no change from production)
SEO: 86 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@milljoniaer milljoniaer marked this pull request as ready for review November 28, 2023 10:54
@milljoniaer milljoniaer requested a review from a team as a code owner November 28, 2023 10:54
Copy link
Member

@georgiee georgiee left a comment

Choose a reason for hiding this comment

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

Hey thanks for finding and fixing this.
Can you share your level of confidence if this will work on production later too?

Regarding the preview url I was able to verify everything works with a link tester tool like https://socialsharepreview.com

gatsby/config-options/constants.ts Show resolved Hide resolved
export const BRANCH_PREVIEW_URL = buildGatsbyCloudPreviewUrl({
prefix: GATSBY_SITE_PREFIX,
export const NETLIFY_DOMAIN_NAME = process.env.GATSBY_NETLIFY_DOMAIN_NAME || '';
export const BRANCH_PREVIEW_URL = buildNetlifyPreviewUrl({
Copy link
Member

Choose a reason for hiding this comment

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

the preview being null is mostly the case for non-preview environments. It will then use GATBSY_BASE_URL (following line). Is this something we create or given by netlify? Asking to make sure this will work when we merge it, because right now I can only verify the preview part of this PR and I fear it's not working later on production. Do you have confidence that this will work then? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GATSBY_BASE_URL (and also the GATSBY_PREFIX which I changed to GATSBY_NETLIFY_DOMAIN_NAME) was part of our environment configuration on gatsby cloud and got lost on the migration to netlify. I readded it in the netlify settings. Hence, it currently works again on production. This PR only fixes the feature branch BASE_URL. for the main branch the BRANCH_PREVIEW_URL will still be "null" and the GATSBY_BASE_URL will be used. And since that works currently on production, I am confident, that it will still work once this PR is merged. But I will monitor it.

@milljoniaer milljoniaer merged commit ed71f56 into main Nov 28, 2023
6 checks passed
@milljoniaer milljoniaer deleted the fix/deployment-base-url branch November 28, 2023 12:31
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.

3 participants