-
Notifications
You must be signed in to change notification settings - Fork 29
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
Final Project || Museek || Alma & Etna #51
base: main
Are you sure you want to change the base?
Conversation
Detail page merge
Museumpage
Feature/modale
Feature/modale
Feature/modale
…e in own component
Frontend/design
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.
Dear Alma & Etna, big congratulations for concluding the final project and Bootcamp with a bang! 💥
I am very impressed on the accuracy of your work, not only the final result is excellent, but the execution too! Achieving these results in only 3 weeks is fantastic!
The app works very well, has a good and tested flow, the UI and UX are well curated too. You nailed responsiveness and accessibility, furthermore in this final project, we asked you to plan your bigger project work, further deepen your knowledge in web development and finally explore new technologies, you surely succeeded in all these aspects!
Talking about the code structure, it is impeccable. You followed the best practices and wrote a clean, readable and formatted code. The backend is solid, well done dividing it into modules to improve scalability and reada 👏 The front-end is also robust, you added an error state component and a lot of nice details to make the app pleasant and easy to use, like the scroll-up functionality when clicking the app logo on the navbar!
I have very little improvements tip to make, but I for example it could be nice to add a loading state while data gets fetched when you click discover more 👌 Also in mobile/tablet screens, remove the 50% width rule and let the list stretch to fill in the screen:
Once again congratulations for your big effort, be proud of yourselves, you proved you’re ready to be graduated developers now ⭐
has_cafe: { type: Boolean }, | ||
website: { type: String }, | ||
opening_hours: [{ type: Schema.Types.Mixed }], | ||
id: { type: 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.
remove the id
field, it gets automatically assign by the database 🧹
frontend/src/util/UrlUtil.js
Outdated
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.
very well using a utils file! A little adjustment: the naming convention in this case is lowercase = utils.js to distinguish them from the Components, and normally it's just called utils 👍
View it live
Frontend: https://museek-project.netlify.app/
Backend: https://museek-2ejb.onrender.com/
Collaborator
[almaherris]