Mini-Challenge 5: Routing#97
Conversation
LuisRamirezHdz
commented
Aug 24, 2021
LuisRamirezHdz
commented
| return ( | ||
| <> | ||
| <h1>You haven't added any video to your favorites yet</h1> | ||
| </> |
There was a problem hiding this comment.
I expected to see this message when going to the favorite's route without adding any favorites but instead I get an empty screen. I think you will need to use your storage and count the items there
| ); | ||
| var path = ''; | ||
| if (route === '/') { | ||
| path = 'video'; |
There was a problem hiding this comment.
I recommend moving both video and favorites to a constant in utils/constants
| fireEvent.click(screen.getByTestId('Logmenu')); | ||
| fireEvent.click(screen.getByText('Cancel')); | ||
| const cancelButton = screen.queryAllByText('Cancel'); | ||
| expect(cancelButton).toHaveLength(0); |
There was a problem hiding this comment.
a way we can improve our tests is to test to what we should be seeing in the screen instead of what is happening "behind the scenes" so instead of using queryAllBy since we only expect 0 or 1 as results we can use queryBy and then we can use not.toBeIntheDocument instead to verify that the user will not be able to see the button
| useEffect(() => { | ||
| if (FavoritesList?.items) { | ||
| //Add or Remove? | ||
| let filterFavorites = FavoritesList.items.filter((e) => e.id.videoId === id); |
There was a problem hiding this comment.
most of the times you're looking for only one element in an array it's more efficient to use .find instead. That returns a boolean so you wouldn't need the .length in the condition below
| } | ||
| }, [FavoritesList, id]); | ||
| function handleClick() { | ||
| if (buttonAction === 'Add') { |
There was a problem hiding this comment.
since you're using 'Add' and 'Remove' in many places in this file. I recommend you use a constant for them to minimize the probability on errors on typos.
const Actions = {
ADD = 'Add',
REMOVE = 'Remove'
}or
const ACTION_ADD = 'Add';
const ACTION_REMOVE = 'Remove';| ); | ||
| } | ||
|
|
||
| export default VideoDetailsView; |
There was a problem hiding this comment.
You have some code that could be reused in FavoriteVideoDetailsView and VideoDetailsView. I recommend instead of two total components you create an extra one that has everything that is shared between these two components and that everything that is different is sent down as props from the two existing components.
Your new component should be the one that is roughly 100 lines long and the existing two should be way shorter as they will only do the things that are particular for them and everything shared is in the new component.
This might benefit from a file renaming as well:
VideoDetailsView -> MainVideoDetailsView
FavoriteVideoDetailsView -> stays the same
new component -> VideoDetailsView
|
@dianaatwizeline , |
2 participants
