-
Notifications
You must be signed in to change notification settings - Fork 2
[State Management] reconsider state management #151
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
Conversation
Snizhana
left a comment
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.
wow, it is so nice to see 800 lines of removed code 😊
package.json
Outdated
| }, | ||
| "scripts": { | ||
| "lint": "eslint --ignore-path .gitignore --ext js,jsx .", | ||
| "lint": "eslint --ignore-path .gitignore --ext js,jsx,tsx .", |
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.
👍 we should probably add .ts as well
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 tend to add this in separate PR, it seems there is a need to add some additional packages in order for lint to work properly with .ts files. Don't want to make a mess here
src/api/index.ts
Outdated
| import { getApiUri } from '../misc/url' | ||
| import { Snippet, RawSnippet } from '../store' | ||
|
|
||
| export const fetchSnippet = (id: number) => { |
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.
nit: we can add a return type to these helpers
src/api/index.ts
Outdated
| body: JSON.stringify(snippet), | ||
| }) | ||
| .then(response => response.json()) | ||
| .then((json) => { |
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.
minor, but there is no need to create an anonymous function .then(onSuccess) should work
src/misc/modes.ts
Outdated
| name: modesByName[item].caption, | ||
| value: item, | ||
| })) | ||
| } No newline at end of file |
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.
minor, a blank line is missing
df71db7 to
fbeb565
Compare
|
hey guys @Snizhana @ikalnytskyi I would highly appreciate some 👀 on recoil usage |
0ac6b8b to
a447993
Compare
ikalnytskyi
left a comment
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 have no idea what Recoil is, but looks better now! 💪
/:snippetIdroute by checking cached snippet by id and if it's not there -> make request