Skip to content

(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

Merged
merged 4 commits into from
Jan 19, 2023

Conversation

davidhou17
Copy link
Collaborator

@davidhou17 davidhou17 commented Jan 11, 2023

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.

  • Create Jira ticket for corresponding docs-app-services update(s), if any
  • Checked/updated Admin API
  • Checked/updated CLI reference

Review Guidelines

REVIEWING.md

@github-actions
Copy link

github-actions bot commented Jan 11, 2023

Readability for Commit Hash: 5cbcb60

You can see any previous Readability scores (if they exist) by looking
at the comment's history.

Flesch Reading Ease scores for changed documents:

  • source/sdk/react-native/realm-database/schemas/embedded-objects: 48.09

The following table can be helpful in assessing the readability score of a document.

Score Difficulty
90-100 Very Easy
80-89 Easy
70-79 Fairly Easy
60-69 Medium
50-59 Fairly Hard
30-49 Hard
0-29 Very Hard

@davidhou17 davidhou17 force-pushed the DOCSP-26993 branch 3 times, most recently from 4f0f1b9 to e359de2 Compare January 17, 2023 18:19
@davidhou17 davidhou17 changed the title WIP - DOCSP-26933: @realm/react-ify 'Embedded Objects' (DOCSP-26933): @realm/react-ify 'Embedded Objects - React Native SDK' Jan 17, 2023
@davidhou17 davidhou17 force-pushed the DOCSP-26993 branch 2 times, most recently from 78f002d to 31444d8 Compare January 17, 2023 18:37
@davidhou17 davidhou17 changed the title (DOCSP-26933): @realm/react-ify 'Embedded Objects - React Native SDK' (DOCSP-26993): @realm/react-ify 'Embedded Objects - React Native SDK' Jan 17, 2023
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()``.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think

Suggested change
``'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()
Copy link
Contributor

@mohammadhunan-dev mohammadhunan-dev Jan 19, 2023

Choose a reason for hiding this comment

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

Suggested change
- 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Contributor

@mohammadhunan-dev mohammadhunan-dev Jan 19, 2023

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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'``
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- 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>
Copy link
Contributor

@mohammadhunan-dev mohammadhunan-dev Jan 19, 2023

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:

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

Copy link
Contributor

@mohammadhunan-dev mohammadhunan-dev left a 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:

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

  2. 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'));
Copy link
Contributor

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')

@davidhou17
Copy link
Collaborator Author

@mongomoe Just made those changes - thanks for the feedback!

@davidhou17 davidhou17 merged commit 142f06f into mongodb:realm-react-guidance Jan 19, 2023
mongodben pushed a commit that referenced this pull request Feb 10, 2023
…#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)
Copy link
Collaborator

@takameyer takameyer left a 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;
Copy link
Collaborator

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.

Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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?

mongodben pushed a commit that referenced this pull request Mar 21, 2023
…#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)
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.

3 participants