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

Consolidation Providers and Hooks #3356

Conversation

osharaf-mdb
Copy link
Contributor

@osharaf-mdb osharaf-mdb commented Aug 5, 2024

Pull Request Info - SDK Docs Consolidation

Jira ticket: https://jira.mongodb.org/browse/DOCSP-41470

Staged Page

Page Source

Note
This PR is built off a previous PR, linked here. The previous PR had feedback from the original draft and the second draft after that. The feedback from that second draft was then implemented to get what is seen here.

PR Author Checklist

Before requesting a review for your PR, please check these items:

  • Open the PR against the feature-consolidated-sdk-docs branch instead of master
  • Tag the consolidated page for:
    • genre
    • meta.keywords
    • meta.description

Naming

Links and Refs

  • Create new consolidated SDK ref targets starting with "_sdks-" for relevant sections
  • Remove or update any SDK-specific refs to use the new consolidated SDK ref targets
  • Update any Kotlin API links to use the new Kotlin SDK roles

Content

  • Shared code boxes have snippets or placeholders for all 9 languages
  • API description sections have API details or a generic placeholder for all 9 languages
  • Check related pages for relevant content to include
  • Create a ticket for missing examples in each relevant SDK: Consolidation Gaps epic

Reviewer Checklist

As a reviewer, please check these items:

  • Shared code example boxes contain language-specific snippets or placeholders for every language
  • API reference details contain working API reference links or generic content
  • Realm naming/language has been updated
  • All relevant content from individual SDK pages is present on the consolidated page

@osharaf-mdb osharaf-mdb force-pushed the react-providers-and-hooks branch 7 times, most recently from 941e596 to ae02b2d Compare August 6, 2024 04:55
Copy link
Collaborator

@krollins-mdb krollins-mdb left a comment

Choose a reason for hiding this comment

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

Nice job, @osharaf-mdb! Thanks for all of the hard work you put into this page! LGTM. 🎊

There are some build errors you should look at though. See the Netlify logs here: https://app.netlify.com/sites/device-sdk/deploys/66b1acaec55e1b00087de096#L405-L423

Copy link

@gagik gagik left a comment

Choose a reason for hiding this comment

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

Great job, great improvements! Some minor comments you could consider:

source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@kneth kneth left a comment

Choose a reason for hiding this comment

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

Nice work! I have added a few comments and suggestions as you can take in if you want


- ``closeOnUnmount?: boolean``

Default is ``true``. If set to ``false``, the open datahase will not close when the
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to mention the scenarios where false is a good value?

source/frameworks/react/providers-hooks.txt Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@dacharyc dacharyc left a comment

Choose a reason for hiding this comment

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

Nice work, Omar! I love it.

I'm requesting changes for a couple of typos I've called out here - we should definitely fix those before merging. I've also left feedback about some more subjective changes, but feel free to take or leave those suggestions - if it's not a typo, it's not blocking.

source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
logInWithAnonymous();
};

* - logInWithApiKey
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 generally ok with the wrapping, but this method where we're wrapping one character just looks really odd to me. I notice a few places below we're wrapping 1, 2, or 3 characters.

I notice some of the code blocks in the table require scrolling even at the current table width. It seems like the // user defined function text is what's requiring most of the code blocks to scroll. I wonder if we can remove that comment - maybe move the comment to the line above or below, or mention above the table that the getAPIKey or getToken functions are user-defined. IF we can remove this user-defined function text, I think we can make the third column a little narrower and give this first column a little more space so we're not wrapping single characters.

source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
source/frameworks/react/providers-hooks.txt Outdated Show resolved Hide resolved
@osharaf-mdb osharaf-mdb force-pushed the react-providers-and-hooks branch 5 times, most recently from 1897b65 to 499ae8b Compare August 8, 2024 20:21
Copy link
Collaborator

@dacharyc dacharyc left a comment

Choose a reason for hiding this comment

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

LGTM - awesome work, @osharaf-mdb ! 🎉 🚀 :shipit:

@dacharyc dacharyc merged commit c2b6210 into mongodb:feature-consolidated-sdk-docs Aug 9, 2024
10 checks passed
@docs-builder-bot
Copy link

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.

6 participants