-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
This includes debouncing user input, querying, and some style stuff.
They'll be added back when we figure out how to implement the artists view.
There are two sources of updates for the search input. 1. The user typing directly into the input. This change comes from a child of the state container. 2. The url changing. This comes from the parent of the state container. There are a few ways to reconcile props from multiple locations, none of which work well. 1. Use key to render a new component when the query changes. This works but the focus of the input is lost. 2. Use componentDidUpdate. It is intended for props and causes an extra render.
Updates flow up from the input to the StateContainer and to the url. State flows down from the url and the state container to the presentational components. This is clear and simple. I think the StateContainer should be simpler.
They both independently handle fetching and rendering of each section of the search page.
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.
Documentation for components would be helpful (just a simple explanation of what the container is trying to do)
</AlbumsQuery> | ||
); | ||
|
||
// TODO: Share Loading More Logic With AlbumsPage |
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.
Is this planned for this PR?
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.
No, planned for #6.
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.
Use doc comments for documentation. (/** */
)
@@ -17,6 +17,9 @@ interface Props { | |||
children: (songs: Song[]) => React.ReactNode; | |||
} | |||
|
|||
// he header for song search results. Handles the cases when `songs` is |
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.
Missing a "T"
When something is called |
As for using doc comments for documentation. Good idea. It is little work for a lot of benefit. I've opened #59 to fix it. |
I didn't name anything |
Implemented the search feature for albums, and songs. Artists will be implemented when the rest of the Artist stuff is done.
Closes #12