-
Notifications
You must be signed in to change notification settings - Fork 88
(DOCSP-26993): @realm/react-ify 'Embedded Objects - React Native SDK' #2486
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
(DOCSP-26993): @realm/react-ify 'Embedded Objects - React Native SDK' #2486
Conversation
Readability for Commit Hash: 5cbcb60 You can see any previous Readability scores (if they exist) by looking Flesch Reading Ease scores for changed documents:
The following table can be helpful in assessing the readability score of a document.
|
4f0f1b9
to
e359de2
Compare
78f002d
to
31444d8
Compare
to create a new ``Address`` embedded object and ``Contact`` parent object based | ||
on the ``TextInput`` values for the contact's name and address. | ||
- Adds an `onPress <https://reactnative.dev/docs/handling-touches>`__ event on the | ||
``'Submit Contact'`` button that calls ``submitContact()``. |
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 don't think
``'Submit Contact'`` button that calls ``submitContact()``. | |
"Submit Contact" button that calls ``submitContact()``. |
Suggestion/not required for LGTM: get rid of the monospace around the "Submit Contact" quote. I prefer not overusing monospace, because it looks odd, so I only really use monospace for APIs, variable names, etc (for instance methods coming from the React API, or @realm/react API) etc.
Feel free to take this suggestion or leave it.
The ``ContactList`` component does the following: | ||
|
||
- Performs a query for all contacts by passing the ``Contact`` class to the ``useQuery`` hook. | ||
- Filters for contacts with the name ``"John Smith"`` by passing :js-sdk:`collection.filtered() |
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.
- Filters for contacts with the name ``"John Smith"`` by passing :js-sdk:`collection.filtered() | |
- Filters for contacts with the name "John Smith" by passing :js-sdk:`collection.filtered() |
Suggestion/Not required for lgtm
- Creates a React `state <https://reactjs.org/docs/react-component.html#state>`__ variable | ||
that represents the contact's new street address. | ||
- Performs a query for all contacts by passing the ``Contact`` class to the ``useQuery`` hook | ||
and filters for the contact that matches the name passed into the component as a prop. |
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.
and filters for the contact that matches the name passed into the component as a prop. | |
and filters for the contact that matches the name passed into the component as a `prop <https://reactjs.org/docs/components-and-props.html>`__. |
Suggestion/not required for LGTM: When it is the first use of a jargony term in a subsection, I like linking out to that term. On subsequent uses of the term, I just use monospace: prop
.
- Creates React `state <https://reactjs.org/docs/react-component.html#state>`__ variables | ||
that represent the contact's new address. | ||
- Performs a query for all contacts by passing the ``Contact`` class to the ``useQuery`` hook | ||
and filters for the contact that matches the name passed into the component as a prop. |
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.
and filters for the contact that matches the name passed into the component as a prop. | |
and filters for the contact that matches the name passed into the component as a `prop <https://reactjs.org/docs/components-and-props.html>__`. |
Suggestion/not required for LGTM
- Gets access to an open realm instance by calling the ``useRealm()`` hook within the component. | ||
- Creates a component method ``deleteContact()`` that performs a write transaction and calls | ||
:js-sdk:`Realm.delete() <Realm.html#delete>` to remove the ``Contact`` object that matches | ||
the name passed into the component as a prop. |
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.
the name passed into the component as a prop. | |
the name passed into the component as a `prop <https://reactjs.org/docs/components-and-props.html>`__. |
Suggestion/not required for LGTM:
- Creates a component method ``deleteContact()`` that performs a write transaction and calls | ||
:js-sdk:`Realm.delete() <Realm.html#delete>` to remove the ``Contact`` object that matches | ||
the name passed into the component as a prop. | ||
- Add an `onPress <https://reactnative.dev/docs/handling-touches>`__ event on the ``'Delete Contact'`` |
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.
- Add an `onPress <https://reactnative.dev/docs/handling-touches>`__ event on the ``'Delete Contact'`` | |
- Add an `onPress <https://reactnative.dev/docs/handling-touches>`__ event on the "Delete Contact" |
Suggestion/not required for LGTM:
}); | ||
}; | ||
return ( | ||
<View> |
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.
Suggestion/not required for LGTM:
Can we:
- Perform the query for
contacts.filtered(`name == '${contactName}'`)[0]
earlier (maybe on line 173), because there's no need for that query to specifically happen when the "Delete Contact" button is pressed. - Add a TextInput that displays the character's name here? That way the theoretical user knows the name of the Contact that they are deleting.
Both of these changes would require a small change to the text/description in the docs under the "Delete an Embedded Object" subsection, and also probably updating the line emphasis if you add additional lines of code here.
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, this looks really good.
Just left some suggestions. Some of the suggestions are reoccurring in a few places:
-
Swapping
”title-of-button”
to “title-of-button” to avoid overusing monospace for something that isn’t an API specific term (such as a method or component name). -
Adding a link for the term ‘prop’ on the first use of the term in the subsection.
const {getByTestId} = render(<App />); | ||
|
||
// test that querying for name works | ||
const contactAddress = await waitFor(() => getByTestId('addressText')); |
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.
Needed fix:
Let's add a timeout to when using the 'waitFor()' method: timeout: 5000. Line 114 includes that timeout.
Having the timeout will prevent the error that we sometimes get without it (I think that error is something like: 'unable to find by testID')
31444d8
to
5cbcb60
Compare
@mongomoe Just made those changes - thanks for the feedback! |
…#2486) ## Pull Request Info ### Jira - https://jira.mongodb.org/browse/DOCSP-26993 ### Staged Changes - [Embedded Objects](https://docs-mongodbcom-staging.corp.mongodb.com/realm/docsworker-xlarge/DOCSP-26993/sdk/react-native/realm-database/schemas/embedded-objects/) ### Reminder Checklist If your PR modifies the docs, you might need to also update some corresponding pages. Check if completed or N/A. - [x] Create Jira ticket for corresponding docs-app-services update(s), if any - [x] Checked/updated Admin API - [x] Checked/updated CLI reference ### Review Guidelines [REVIEWING.md](https://github.com/mongodb/docs-realm/blob/master/REVIEWING.md)
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.
Mostly good, but I am concerned about the snippets being untested code. Also have some suggestions.
// Run the `.filtered()` method on all the returned Contacts to find the | ||
// contact with the name "John Smith" and the corresponding street address | ||
const contactAddress = contacts | ||
.filtered("name == 'John Smith'")[0].address.street; |
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.
This would cause a crash if 'John Smith' didn't exist, and I'm also quite sure this would show a type error.
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.
Also want to point out, we have many untested snippets here. Why wouldn't the source code be derived from embedded-objects-tests
?
Query a Collection on Embedded Object Properties | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
You can use dot notation to filter or sort a :ref:`collection |
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.
The example below doesn't actually do this. It just does a query on the parent property and accesses an embedded object in the results. I think we would want to document how to actually build a filter based on embedded data.
|
||
Delete an Embedded Object | ||
~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
Realm Uses Cascading Deletes for Embedded Objects. To delete an embedded object, | ||
delete the embedded object's parent. | ||
|
||
.. literalinclude:: /examples/generated/node/data-types.snippet.delete-an-embedded-object.js | ||
:language: javascript | ||
Example |
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.
How about deleting an embedded object without deleting the parent?
…#2486) ## Pull Request Info ### Jira - https://jira.mongodb.org/browse/DOCSP-26993 ### Staged Changes - [Embedded Objects](https://docs-mongodbcom-staging.corp.mongodb.com/realm/docsworker-xlarge/DOCSP-26993/sdk/react-native/realm-database/schemas/embedded-objects/) ### Reminder Checklist If your PR modifies the docs, you might need to also update some corresponding pages. Check if completed or N/A. - [x] Create Jira ticket for corresponding docs-app-services update(s), if any - [x] Checked/updated Admin API - [x] Checked/updated CLI reference ### Review Guidelines [REVIEWING.md](https://github.com/mongodb/docs-realm/blob/master/REVIEWING.md)
Pull Request Info
Jira
Staged Changes
Reminder Checklist
If your PR modifies the docs, you might need to also update some corresponding
pages. Check if completed or N/A.
Review Guidelines
REVIEWING.md