Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Compare since previous compare PR #4
base: initial
Are you sure you want to change the base?
Compare since previous compare PR #4
Changes from 10 commits
3ecc9ef
b947804
5119278
ad77205
69a671d
627dc8f
233b7e8
eb84b11
812d456
3b2d7e2
b0cd8b4
aaeaec0
cf1bde7
660977c
b494a52
4dbcfec
22e7c24
ae941bd
11c9a6e
ca323bb
72db333
631ef8a
4a263fc
41a8b2e
8855891
7620d10
ac1ea6c
b06a3d4
4fa90a9
763e61f
948eaab
8413354
b18ed6a
b0b6b80
9390c2d
b9c09d0
4b48b2c
5842ae0
b949d42
7676ef0
70206b1
dff866e
371de77
2b25ef6
8efdec6
524408f
571c1b0
bc5af78
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 have all of the components defined inside of this
App
component, as well as the creation of the test data happening inside of the component. This has a side effect of making it so React has no context of when things "change" here, or when they have "stayed the same" between renders.The reference to the child component definitions are changing every time, and the data being passed in as props (at least they likely will be later). For minimal refactoring to improve this, I recommend the following making the following changes:
Create
types.ts
Create
testdata.ts
Then in
App.tsx
We define components like
Files
outside of the definition ofApp
. Ideally these are defined in their own files, but keeping them in the same file asApp
is less work for now. Separating the definitions to be on the module-level of the file is important though.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.
Okay, so do you want me to move the components to their own files?
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.
Yeah each of the components seem separate enough to warrant living in their own individual files
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 vscode and typescript make it really easy to move components into other files. You can highlight the text of the component, then right-click and select "Refactor..." (I prefer to use the command palette and type "refactor" instead of right-clicking), then select "Move to a new file". It generates a new file, which you can then rename and relocate as necessary. vscode will automatically:
You don't have to do any of these things. It just does it for you. It's called "refactor" here since the change is made cleanly, and the functionality of the program doesn't change
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.
Yeah man, I love the refactor command! It's so useful
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.
It probably makes sense to have the submit button trigger an update to the array of comments shown. That would make the code more ready for porting over.
We would add a prop to this
Submit
component for the parent component to process the form submission.Also, I'm thinking this component can be called
CreateComment
to be more specific about what it's doing. We can then have the passed in callback prop to be calledonSubmit
. What do you think?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 think that renaming the component to CreateComment is a good idea for sure, since the name is now more clear as to its purpose.
I believe that in order for this component to update the array of comments shown, there are a few options we could go for, namely:
What are your thoughts on this?
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'm thinking localStorage will be useful to help with mocking the actual backend now. Probably good idea to have a client.ts file that acts like we're fetching and saving to the backend. That will handle all localStorage things behind the scenes. Here,
client
refers to "http client" in a way. It's our gateway to the backend's endpoints, but we can fake the client's implementation with localStorage for now.After this we can have a browse project page that will help show other aspects of the UI. It would be a view where you can select a given section in a project. Then we could also have a projects browser that allows you to see a list of projects. What do you think?