-
Notifications
You must be signed in to change notification settings - Fork 88
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
Consolidation Providers and Hooks #3356
Conversation
941e596
to
ae02b2d
Compare
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.
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
ae02b2d
to
ccaec73
Compare
ccaec73
to
3c4d085
Compare
926f4d1
to
edc9400
Compare
edc9400
to
002fa93
Compare
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.
Great job, great improvements! Some minor comments you could consider:
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.
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 |
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 want to mention the scenarios where false
is a good value?
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.
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.
logInWithAnonymous(); | ||
}; | ||
|
||
* - logInWithApiKey |
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 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.
1897b65
to
499ae8b
Compare
499ae8b
to
a77af23
Compare
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.
LGTM - awesome work, @osharaf-mdb ! 🎉 🚀
c2b6210
into
mongodb:feature-consolidated-sdk-docs
✨ Staging URL: https://preview-mongodbmongodb.gatsbyjs.io/realm/feature-consolidated-sdk-docs/ 🪵 Logs |
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:
feature-consolidated-sdk-docs
branch instead ofmaster
Naming
.rst
files comply with the naming guidelinesLinks and Refs
Content
Reviewer Checklist
As a reviewer, please check these items: