Skip to content

Todo list implementation#1

Open
ankit-agrawal11 wants to merge 8 commits intodevfrom
todo-implementation
Open

Todo list implementation#1
ankit-agrawal11 wants to merge 8 commits intodevfrom
todo-implementation

Conversation

@ankit-agrawal11
Copy link
Copy Markdown
Contributor

@ankit-agrawal11 ankit-agrawal11 commented Aug 1, 2018

Title

Todo list implementation

Problem

Ramping up react native.

Solution / Approach

Creating a react native app to learn the basics.

Dependencies?

None

Test Procedures

Clone the repo and then run:
npm i
exp start (You may need to install expo)

Download expo app from play store and scan the QR code.
Test the basic Todo implementation

Additional comments / concerns

None

@ankit-agrawal11
Copy link
Copy Markdown
Contributor Author

Comment thread README.md
@@ -0,0 +1 @@
A react native application with todo implementation.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We generally add TODO now you have implemented. 😅

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

😅

Copy link
Copy Markdown

@shivamkrpandey shivamkrpandey left a comment

Choose a reason for hiding this comment

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

Test it with iOS too.

Comment thread tsconfig.json Outdated
// "experimentalDecorators": true, /* Enables experimental support for ES7 decorators. */
// "emitDecoratorMetadata": true, /* Enables experimental support for emitting type metadata for decorators. */
}
} No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove unnecessary properties.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ankit-agrawal11 ankit-agrawal11 changed the title [WIP] Todo list implementation Todo list implementation Aug 2, 2018
@ankit-agrawal11
Copy link
Copy Markdown
Contributor Author

Comment thread src/App.tsx Outdated
marginTop: 5,
marginBottom: 5
},
listItemCont: {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Did you install spell-check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. spell-check didn't show an error for this. 😅
Fixed.

Comment thread src/App.tsx Outdated

Keyboard.addListener(
isAndroid ? "keyboardDidHide" : "keyboardWillHide",
() => this.setState({ viewPadding: viewPadding })
Copy link
Copy Markdown

@parul-causecode parul-causecode Aug 24, 2018

Choose a reason for hiding this comment

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

Can Use shorthand

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

Comment thread src/App.tsx
);

const tasks = await this.getTasksFromStorage()
this.setState({tasks});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

setState only once, line 36,41 and 45 can be combined as
this.setState({ viewPadding: someVarHoldingTheValue, tasks })

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Setting different padding when the keyboard is visible and when it's hidden. So, this can not be combined.

Comment thread src/App.tsx
render(): JSX.Element {
return (
<View style={[styles.container, { paddingBottom: this.state.viewPadding }]}>
<FlatList
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

list and text inputs can be broken down into shorter renders.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That would look weird. Since this view only renders FlatList and TextInput and there is no logic. I am just passing the props.
renderItem prop's implementation has been moved to a separate function. That should make it readable now.

Comment thread src/App.tsx
testID='flat-list'
style={styles.list}
data={this.state.tasks}
renderItem={(task: { index: number, item: { text: string, key: string, isCompleted: boolean } }) =>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

renderItem can move out as a separate function, it's for the ease of debugging

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

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