Skip to content
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

MovieSearchApp #657

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

jaroslav-cabala
Copy link

@jaroslav-cabala jaroslav-cabala commented Nov 15, 2024

screen-capture.webm

My take on the Movie Search Application.

I chose to render movie details in a dialog instead of creating a separate route for the details view. For an app of this size I find it much better user experience :)

If I had more time I would definitely fix width of table columns. I used Shadncn/ui table and unfortunetaly the width changes based on the length of the movie title I guess which is visible when changing pages. Also, the Shadncn/ui dialog component bleeds out of the container when zoomed too much. Again, default behaviour of the component(I tried it on the Shadncn page and it suffers from the same problem 🤷). And styling might need some love...(I am not a css master so I did what I could 😃).

I also wrote tests for 2 main components, MovieSearch and MovieDetails. We could write more tests, for example pagination in this app, but in a real project we would not need to write the pagination ourselves. And also instruction say that I don't need to write 200 unit test :) Tests I wrote are important though and should be always included. They test what the user is supposed to see and be able to do.
In a real production code I would definitely include some integration/e2e tests that would test things unit test don't/shouldn't.

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.

1 participant