-
Notifications
You must be signed in to change notification settings - Fork 176
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
docs(next-drupal): updated docs #787
docs(next-drupal): updated docs #787
Conversation
Someone is attempting to deploy a commit to the Chapter Three Team on Vercel. A member of the Team first needs to authorize it. |
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.
Doing a review in phases here, because this is a big PR :) I'm through the root of docs, up to the reference section.
So far this looks great @MontiMarco92
There are a few things I think we'll have to flesh out beyond the changes here (although I may not have gotten to it yet in the PR):
- Upgrading from V1 to V2
- A home for NextDrupalPages specific content.
- Some additional detail related to caching and the app router.
But I'm thinking we should be able to get these changes merged as a first step.
Hope to have some time to continue the review soon...
) | ||
export const drupal = new NextDrupal(process.env.NEXT_PUBLIC_DRUPAL_BASE_URL, { | ||
fetcher: customFetcher, | ||
}) | ||
``` |
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.
Based on the comment above, I think we should add a callout here, explaining that if you provide a custom fetcher, you'll need to manage fetch data caching manually using the unstable_cache API.
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.
See comment above
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.
Sharing another round of review comments - about halfway through.
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.
Finished a full pass at review here. Excellent work @MontiMarco92 !
Next steps:
- Incorporate the requested revisions.
- Find a home for NextDrupalPages/Page Router specific content.
- Come to a conclusion about how we handle things related to caching and the app router - do we remove for now, or do we integrate feat(next-drupal): next revalidate options #784 before we merge this.
- Add a section on upgrading from V1 to V2 (the changelog in the codebase probably gives us a start here)
Happy to have your continued help here @MontiMarco92, but don't expect you to cover all of this. As a result, we'll need to come up with some way to collaborate on this PR.
www/content/docs/reference/getresourcecollectionpathsegments.mdx
Outdated
Show resolved
Hide resolved
www/content/docs/reference/getresourcecollectionfromcontext.mdx
Outdated
Show resolved
Hide resolved
www/content/tutorials/on-demand-revalidation/configure-entity-types.mdx
Outdated
Show resolved
Hide resolved
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'm not really sure of the best way to handle this, but technically this content will still be relevant to those using the pages router. Maybe this could be included as an additional section on the configure draft routes entry?
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 haven't addressed this, cause I think It has to be made a decision regarding the next drupal pages specific documentation.
Thanks a lot for your review @backlineint and your feedback! I'll be happy to address whatever changes I can when I have time.
Thanks again and I hope this will help |
@backlineint I have addressed some of the requested modifications. I've left some comments as well.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Thanks for making these updates @MontiMarco92! I'm approving this so we can merge the current state. That will make it easier to collaborate on the remaining adjustments (I'm planning on creating a new feature branch on the main remote.) It will also allow us to deploy the current state to https://next.next-drupal.org/
@MontiMarco92 I created a Draft PR with a start of the final adjustments we need: #804 The currently merged state of these docs are now available on https://next.next-drupal.org/ |
This pull request is for: (mark with an "x")
examples/*
modules/next
packages/next-drupal
starters/basic-starter
starters/graphql-starter
starters/pages-starter
GitHub Issue: #691
Code changes need test coverage. If you don't know
how to make tests, check this box to ask for help.
Describe your changes
I've updated the documentation to reflect the changes of the new 2.0 version. Basically I've updated all the pages related to the following sections on the
/docs/
route.I followed the same structure and did not add extra sections (perhaps some other additional example).
On the
Reference
section I've removed the old methods and added some new ones. The complete list of the updated methods are:Methods that have not been added:
Let me know if it's relevant that they are added as well
Note: On some of them, I assumed the
next
revalidation options proposed on this PR will be accepted. However, it can be the case that this feature is modified and hence, the docs will have to be updated as well. Besides, not all of the methods have been extended to take this newnext
option as explained on the PR, since I'm waiting for feedback.Note 2: I've left some HTML comments on some places just to note that there are some questions on the matter (at least from me).
EDIT: I've updated the "Learn" section as well. There might be changes on the new drupal module that are not reflected .
Hope this helps!
Contribution
Sponsored by @brainsum