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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MontiMarco92
Copy link

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

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

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
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
Contributor

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

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

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
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
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
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.

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