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

docs(next-drupal): updated docs #787

Merged

Conversation

MontiMarco92
Copy link
Contributor

@MontiMarco92 MontiMarco92 commented Jul 9, 2024

This pull request is for: (mark with an "x")

  • examples/*
  • modules/next
  • packages/next-drupal
  • starters/basic-starter
  • starters/graphql-starter
  • starters/pages-starter
  • Other

GitHub Issue: #691

  • I need help adding tests. (mark with an "x")
    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.

  • Drupal Client
  • Working with JSON:API
  • Building Pages
  • Customization
  • Reference

image

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:

  • getResource
  • getResourceByPath
  • getResourceCollection
  • createResource
  • createFileResource
  • updateResource
  • deleteResource
  • getResourceCollectionPathSegments
  • translatePath
  • constructPathFromSegment
  • buildEndpoint (not sure if it's intended to be used by users or only internally)
  • getAccessToken
  • getMenu
  • getView
  • getSearchIndex
  • buildUrl
  • fetch
  • deserialize
  • getAuthorizationHeader

Methods that have not been added:

  • fetchResourceEndpoint
  • addLocalePrefix
  • validateDraftUrl

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 new next 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

Copy link

vercel bot commented Jul 9, 2024

Someone is attempting to deploy a commit to the Chapter Three Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Collaborator

@backlineint backlineint left a 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...

www/content/docs/client.mdx Outdated Show resolved Hide resolved
www/content/docs/cache.mdx Outdated Show resolved Hide resolved
www/content/docs/cache.mdx Outdated Show resolved Hide resolved
www/content/docs/authentication.mdx Outdated Show resolved Hide resolved
www/content/docs/client.mdx Outdated Show resolved Hide resolved
)
export const drupal = new NextDrupal(process.env.NEXT_PUBLIC_DRUPAL_BASE_URL, {
fetcher: customFetcher,
})
```
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above

www/content/docs/fetching-resources.mdx Outdated Show resolved Hide resolved
www/content/docs/pages.mdx Outdated Show resolved Hide resolved
www/content/docs/pages.mdx Outdated Show resolved Hide resolved
www/content/docs/pages.mdx Outdated Show resolved Hide resolved
Copy link
Collaborator

@backlineint backlineint left a 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.

www/content/docs/reference/buildEndpoint.mdx Outdated Show resolved Hide resolved
www/content/docs/reference/buildEndpoint.mdx Outdated Show resolved Hide resolved
www/content/docs/reference/getentryforresourcetype.mdx Outdated Show resolved Hide resolved
www/content/docs/reference/getmenu.mdx Outdated Show resolved Hide resolved
www/content/docs/reference/getmenu.mdx Show resolved Hide resolved
www/content/docs/reference/getpathfromcontext.mdx Outdated Show resolved Hide resolved
www/content/docs/reference/getresource.mdx Outdated Show resolved Hide resolved
www/content/docs/reference/getresourcebypath.mdx Outdated Show resolved Hide resolved
www/content/docs/reference/getresourcecollection.mdx Outdated Show resolved Hide resolved
Copy link
Collaborator

@backlineint backlineint left a 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/getresourcefromcontext.mdx Outdated Show resolved Hide resolved
www/content/docs/reference/getsearch.mdx Outdated Show resolved Hide resolved
www/content/docs/reference/getstaticpathsfromcontext.mdx Outdated Show resolved Hide resolved
www/content/docs/serializer.mdx Outdated Show resolved Hide resolved
www/content/tutorials/draft-mode/create-site.mdx Outdated Show resolved Hide resolved
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

www/content/tutorials/quick-start/index.mdx Outdated Show resolved Hide resolved
@MontiMarco92
Copy link
Contributor Author

Thanks a lot for your review @backlineint and your feedback! I'll be happy to address whatever changes I can when I have time.

  • If by the time I come back on this, the conclusion on the caching issue feat(next-drupal): next revalidate options #784 has not yet being defined I will remove all the things that reference that on this PR.
  • I might further ask your opinion/directive on how/where to locate the NextDrupal pages router docs.

Thanks again and I hope this will help

@MontiMarco92
Copy link
Contributor Author

@backlineint I have addressed some of the requested modifications. I've left some comments as well.

  • Regarding the previously removed files that are still valid for the NextDrupalPages client, I've restored them, but haven't referenced them on the menu. They should be referenced once you know where this client specific docs are going to be placed.

  • I've removed all the revalidation related references since a decision regarding the caching system feat(next-drupal): next revalidate options #784 needs to be made.

Copy link

vercel bot commented Oct 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-drupal-next ❌ Failed (Inspect) Oct 11, 2024 1:41pm

Copy link
Collaborator

@backlineint backlineint left a 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/

@backlineint backlineint merged commit f377da8 into chapter-three:main Oct 11, 2024
3 of 17 checks passed
@backlineint
Copy link
Collaborator

@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/

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.

2 participants