Skip to content

Conversation

berthieresteban
Copy link
Contributor

@berthieresteban berthieresteban commented Jan 9, 2020

What does this PR do?

This PR add the react native getting started project with cypress tests.
Like the others getting started this a simple chat with document create and realtime subscription.
The readme will follow in another PR.

⚠️
In the screens/Chat/ChatView.js file, each message only display the author, the messge and the hour. The date of the message is displayed once for each message sended on the same date.

I'm not sure about the fetchMessages() subscribeMessages() and getMessage() functions in screens/Chat/ChatClient.js file because of the little complexity of the code at the top of the getMessage() function.
It's just used to check on each message if the date need to be displayed by checking the previous date message.
It also decrease the readability of the render() function of the screens/Chat/ChatView.js file.
Please tell me if you think it's too much for a simple getting started or if I need to simplify it.
⚠️

How should this be manually tested?

  • Step 1 : npm i
  • Step 2 : cd ./doc/7/getting-started/.react-native
  • Step 3 : npm i
  • Step 4 : npm run web
    In another term from the same folder:
  • Step 5 : npm run test (if you have the CYPRESS_RECORD_KEY env variable setted)
  • Step 5bis : node_modules/.bin/cypress run (else)

Other changes

/

Boyscout

In test-docs.sh, use npm install command instead of npm ci because we must use the builded sdk of this repo instead of the one installed with package.json. (cf: package.json -> scripts -> postinstall)

Rename spec files of cypress tests for a better readability on dashboad.cypress.io

@codecov
Copy link

codecov bot commented Jan 9, 2020

Codecov Report

Merging #480 into 7-dev will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            7-dev     #480   +/-   ##
=======================================
  Coverage   95.57%   95.57%           
=======================================
  Files          32       32           
  Lines        1312     1312           
=======================================
  Hits         1254     1254           
  Misses         58       58           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84a59f0...11da4d6. Read the comment docs.

@alexandrebouthinon alexandrebouthinon changed the title add gs react native and tests and ci Add React Native getting started Jan 13, 2020
berthieresteban and others added 2 commits January 22, 2020 10:31
This PR add the snippets and the Readme of the react native getting started.
);
} else {
return (
<Login onSubmitName={this.handleSubmitName} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not directly pass this.onSubmitName here?

Copy link
Member

Choose a reason for hiding this comment

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

Because function need to be bounded to the context of this class in order to use the this keyword inside the login component.

But i agree that this.handleSubmitName = this.onSubmitName.bind(this); should not be named differently to keep readability of the component. They both should keep the name handleSubmitName

@@ -0,0 +1,149 @@
/* snippet:start:1 */
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation of this file is wrong (4 spaces instead of 2)

render() {
const messages = this.state.messages;
return (
<ChatView messages={messages} onSendMessage={this.handleSendMessage} name={this.props.name} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark, why don't pass directly this.onSendMessage ?

@@ -0,0 +1,135 @@
/* snippet:start:1 */
Copy link
Contributor

Choose a reason for hiding this comment

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

This file have a wrong indentation (4 spaces instead of 2)

<TextInput
style={styles.inputMessage}
placeholder='Send message...'
onSubmitEditing={this.handleSendMessage}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark, why don't just pass this.onSendMessage instead?

@@ -0,0 +1,40 @@
/* snippet:start:1 */
Copy link
Contributor

Choose a reason for hiding this comment

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

This file have a wrong indentation (4 spaces instead of 2)

);
} else {
return (
<Login onSubmitName={this.handleSubmitName} />
Copy link
Member

Choose a reason for hiding this comment

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

Because function need to be bounded to the context of this class in order to use the this keyword inside the login component.

But i agree that this.handleSubmitName = this.onSubmitName.bind(this); should not be named differently to keep readability of the component. They both should keep the name handleSubmitName

<View>
<View>
{
item.displayDate ?
Copy link
Member

Choose a reason for hiding this comment

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

This could be written item.displayDate && .... instead of item.displayDate ? ... : null

return (
<SafeAreaView style={{ flex: 1 }}>
<KeyboardAvoidingView style={styles.container} behavior="padding">
<FlatList
Copy link
Member

@rolljee rolljee Feb 4, 2020

Choose a reason for hiding this comment

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

For readability purpose you shall put the component inside a dedicated function ?
maybe a function called renderFlatList() {}

Copy link
Contributor

@Aschen Aschen left a comment

Choose a reason for hiding this comment

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

After the failing test pass, 👍

@Aschen Aschen merged commit e590c90 into 7-dev Feb 19, 2020
@Aschen Aschen deleted the add-getting-started-react-native branch February 19, 2020 13:50
@Yoann-Abbes Yoann-Abbes mentioned this pull request Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants