-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add delete experiences #36
Conversation
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.
Status: Approved 🟢 💯
Hi @HFG43 & @NitBravoA92
Great work on implementing the missing feature, this is complete so it's time to merge it 💯 🥳.
Highlights 🥇
- Linters running
- The app allows to create and delete experiences
- Good design
Optional
All the changes with optional tags are not crucial enough to prevent you from getting the approval. Still, we would highly recommend that you consider these changes.
Happy coding! 👏 👏 👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear. Remember to tag me in your question so I can receive the notification.
src/Components/AddExperience.jsx
Outdated
<input id="testing" type="number" placeholder="Additional Testing Fee" className="form-input reserv-input" required min="0.01" step="0.01" /> | ||
<input id="fulltable" type="number" placeholder="Additional Full Table Fee" className="form-input reserv-input" required min="0.01" step="0.01" /> | ||
<input id="guests" type="number" placeholder="Guests included in Experience" className="form-input reserv-input" required min="1" step="1" /> | ||
<input id="image" type="text" placeholder="Add an Experience image" className="form-input reserv-input" required /> |
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.
[Optional] You did an excellent job on the feature, I suggested you use a more descriptive placeholder on the image input, specifying that it is a URL image.
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.
@AndreaM2429 Thank you so much for giving us this valuable suggestion! The changes have been applied.
🚩Required improvements in the React App
Hi @Dachrono and @AndreaM2429
We're sending this pull request for you to review my code. Thanks in advance for your feedback!
Milestones 🎯:
📌 Update Component functions to use ES6 arrow functions
📌 Create the AddExperience component with the form to add an experience
📌 Create the DeleteExperience component to display all the experience and allows the user to remove them
📌 Update the NavBar and the app routes to add two new links:
/experiences/new
and/experiences/delete
📌 Refactor the functionality to
LogOut
⭐To the code reviewer 👨💻
Please, I kindly ask you to review the entire code and verify that all requirements have been met correctly.
if there is any issue 🦯 in this PR, please do list 📃 them in a descriptive 💡 manner and give your best suggestions 🎁 if needed.