-
Couldn't load subscription status.
- Fork 1
Space Travellers' Hub: Rocket #199
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
base: dev
Are you sure you want to change the base?
Conversation
Basic Dependency Installation
Nav Setup
Style : Main Page
Testing : Rockets Components
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.
Hi @amrendrakind 👋 ,
Good job so far working on the project requirements!
There are some issues that you still need to work on to go to the next project but you are almost there!

To Highlight 🎯
- No linter errors. ✔️
- GitHub flow is followed. ✔️
- Good Pr title/description ✔️
- Good commit history ✔️
Required Changes ♻️
Kindly fix the issues described in the pr thread below in order to get an approval
Optional suggestions
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
Cheers and Happy coding!clapclapclap
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me @Ol-create in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
Happy coding! 🎉 🎉
| import logo from './logo.svg'; | ||
| import React from 'react'; | ||
| import { Route, Routes } from 'react-router-dom'; | ||
| import './App.css'; | ||
| import NotFound from './components/pages/NotFound'; | ||
| import Dragons from './components/dragon/dragons'; | ||
| import MissionsContainer from './components/pages/MissionsContainer'; | ||
| import Rockets from './components/rockets/Rockets'; | ||
| import NavBar from './components/navbar/NavBar'; | ||
| import ProfileContainer from './components/pages/ProfileContainer'; |
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.
- Please take into consideration the following suggestions to enhance the readability of your code:
-
Group the imports: Organize the imports into logical groups based on their source or functionality. For example, you can have separate groups for built-in modules, third-party libraries, and local project files.
-
Alphabetize the imports within each group: Within each import group, arrange the imports in alphabetical order based on their source. This approach provides a consistent and predictable ordering for easy reference.
By implementing these recommendations, your code will become more structured and easier to navigate.
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.
Thank you for your suggestions to enhance the readability of my code. 💯
| const DragonElement = (props) => { | ||
| const dispatch = useDispatch(); | ||
| const { | ||
| id, name, description, flickrImages, reserved, | ||
| } = props; |
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.
- Please consider to destructure the props directly in the function signature.
To improved readability and avoiding repetition:+1:
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.
I appreciate your advice and have implemented it in this approach to enhance the readability and maintainability of my code. Thank you for your valuable suggestion. 👍
| DragonElement.defaultProps = { // Modifying defaultProps inside the component is not recommended | ||
| reserved: false, | ||
| }; | ||
|
|
||
| const reserveDragonHandler = (e) => { | ||
| if (props.reserved) { // Accessing props directly in the handler is not efficient | ||
| dispatch(DragonsActions.unreserveDragon(e.target.id)); | ||
| } else { | ||
| dispatch(DragonsActions.reserveDragon(e.target.id)); | ||
| } | ||
| }; | ||
|
|
||
| const reservation = props.reserved ? 'Cancel Reservation' : 'Reserve Dragon'; // Accessing props directly | ||
| const btnClass = props.reserved ? 'grayBtn' : 'blueBtn'; // Accessing props directly | ||
|
|
||
| return ( | ||
| <div className="dragonEl"> | ||
| <div className="dragonImg"> | ||
| <img src={flickrImages} alt={description} /> | ||
| </div> | ||
| <div> | ||
| <h2>{name}</h2> | ||
| <p> | ||
| {props.reserved && <span className="badge">Reserved</span>} {/* Using unnecessary parentheses */} | ||
| {description} |
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.
I suggest that you remove unnecessary comments, to Improved code readability and avoiding confusion and misinformation 🎉 🎉
src/components/dragon/JoinButton.js
Outdated
| const JoinButton = (props) => { | ||
| const { dragon } = props; | ||
| const { id, reserved } = dragon; |
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.
- Please destructured the dragon prop directly in the function signature.
To improved readability and avoiding repetition 👍
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.
Thank you for your valuable suggestion.
I have implemented it 👍
| JoinButton.propTypes = { | ||
| dragon: | ||
| PropTypes.objectOf( | ||
| { | ||
| id: PropTypes.number, | ||
| reserved: PropTypes.string, | ||
| }, | ||
| ).isRequired, | ||
| }; |
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.
- Please consider removing unnecessary nesting in the propTypes definition.
By removing unnecessary nesting, you make the propTypes definition more straightforward and easier to comprehend. It allows developers to quickly identify and understand the expected props for a component without having to navigate through unnecessary layers of nesting. 🎉 🎉
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.
I appreciate your advice
I have implemented in this approach to enhance the readability and maintainability of my code.
Thank you for your valuable suggestion. 🥇
| {dragonsList.map((dragon) => ( | ||
| <DragonElement | ||
| key={dragon.dry_mass_kg} | ||
| id={dragon.id} | ||
| name={dragon.name} | ||
| description={dragon.description} | ||
| flickrImages={dragon.flickr_images[0]} | ||
| reserved={dragon.reserved} | ||
| /> |
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.
- Please consider destructuring the necessary properties (id, name, description, flickr_images, reserved) directly
from the dragon object, to improved readability and avoiding repetition 👍
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.
I appreciate your advice
I have implemented in this approach to enhance the readability and maintainability of my code.
Thank you for your valuable suggestion. 🥇 🥇
| <ul className="nav-links"> | ||
| {links.map((link) => ( | ||
| <li className="nav-link" key={link.id}> | ||
| <NavLink to={link.path}> |
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.
- Please add the activeClassName prop to the NavLink component
to specify the class name for the active link. 👍
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.
Thank you buddy 👍
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 🍾 ✔️🎊
Congratulations! 🎉 Project Approved ✅
Good job so far. Well Done.👍🏻
Your project is complete! There is nothing else to say other than... it's time to merge it ![]()

To Highlight 🎉
- No linter errors. ✔️
- Git flow is followed. ✔️
- You have followed JavaScript best practices. ✔️
- Professional README file. ✔️
- PR has a good title and summary. ✔️
- Your app works as expected 💯 🥇
- Your Design looks awesome, keep rocking 🚀
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers 🥂 and Happy coding!!! 👯💻
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please ping me @Ol-create when you comment so I can receive the notification.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
|
Thank you @Ol-create 👍 |
Space Travellers' Hub
Created the homepage leading to the "Rockets" section
Created a "Missions" sections
Created a "Dragons" sections
Created a "My Profile" section
All the information is taken from the provided APIs