-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Update buttercms example #35436
Update buttercms example #35436
Conversation
@leerob Hello! Sorry for reaching out directly, but I think Jake mentioned he'd corresponded with you previously about updating our Butter starter project. I just wanted to check on this. We're so excited to get this new starter merged in :). Could you let me know if there's some action I need to take? Thank you :) |
@leerob I've made the changes asked for, but have not pushed up yet as I wanted to check - would you like us to squash our commits into one and rebase on top of the most recent canary? |
You don't need to squash 👍 Whatever works for you |
@leerob i believe this is ready for review as the changes requested have been made. One note: I did run |
@leerob I see that there is a failing test here, however, I don't see anything to fix in the error message? Would it be possible for you to let us know what we need to do from here? Thank you, and sorry for the inconvenience! |
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.
Left a few more comments, looking good! Thanks for making those changes.
examples/cms-buttercms/.env.sample
Outdated
@@ -0,0 +1 @@ | |||
NEXT_PUBLIC_BUTTER_CMS_API_KEY=your_auth_token |
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 think we should keep this as .env.local.example - any reason it changed?
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.
reverted back to .env.local.example
examples/cms-buttercms/.gitignore
Outdated
@@ -32,3 +32,6 @@ yarn-error.log* | |||
|
|||
# vercel | |||
.vercel | |||
|
|||
# other | |||
.vscode |
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.
The test was failing because of this - all examples need to have the same .gitignore 👍
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've committed this change
}, | ||
images: { | ||
domains: ['cdn.buttercms.com'], | ||
dangerouslyAllowSVG: true, |
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.
Do we need this?
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 believe we need the domains specification as the images for the landing page come from the CDN, but I've deleted the dangerously allow SVG key
@@ -1,28 +0,0 @@ | |||
import { getPreviewPostBySlug } from '@/lib/api' | |||
|
|||
export default async function preview(req, res) { |
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 reason this was removed? It'd be great if it used on-demand ISR as well 😄
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.
My understanding is that we no longer support the same method for previewing of a draft post, so there's no longer a getPreviewPostBySlug method.
As for the on-demand ISR - that sounds like a very cool feature! Unfortunately, the main next.js developer that we had for creating the starter is not available, and I'm really hesitant to change anything that's going to affect functionality on that level, as I'm not a next.js developer (or even js developer 😅 myself).
We have another version of this starter in our docs and our page that we'd also have to update, as we're trying to keep the two versions (i.e., the one in our docs and the one in the next.js examples) from diverging too greatly. Our main goal with the starter was for someone to be able to set it up super-rapidly using the exact same steps across all stacks that we support, so we'd like to prevent having to set up extra environment variables, etc.
If the on-demand static ISR is an absolute requirement for updating our current example, I can forward that information to my team, but I think our preference at this time would be to not implement it if that's a possibility? It also makes the process of testing/hosting a bit more difficult, due to the need to create a production build and start the production server, and we're trying to keep these starters as easy to drop in as possible.
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.
No worries - could be a future improvement!
Co-authored-by: Lee Robinson <me@leerob.io>
@leerob I think I've addressed all the changes from this last batch, save regarding the on-demand ISR; please see my note. Please let me know if there's anything I forgot or need to do, and thank you again. |
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.
Thank you so much for dealing with my changes!
Of course - and thank you for your feedback. It was extremely clear and implementable; much appreciated :) |
@leerob I see we have a failing test again, sorry 😢. Please let me know what I can do to fix this, and thanks again for all your help. |
@leerob Oh, awesome! So exciting to see all tests pass! Thank you! |
Documentation / Examples
yarn lint
Note: I was having a lot of linting errors on a freshly pulled down copy of the canary branch (see code snippet at bottom). After consulting with my team, I checked out the UPDATE_BUTTERCMS_EXAMPLE branch and ran yarn lint-no-typescript (our project is not typescript), which came back with two errors for my new branch, one of which was easily fixed. The other is this:
If i remember correctly, this author card component was originally set up with
<Image />
, but was changed to a<img>
because of the need for people to choose author profile images from whatever domain they wanted, as people have the option to set their profile image in butter to any image (e.g., outside of our cms).If this is not acceptable, please advise how you would like us to address.