-
Notifications
You must be signed in to change notification settings - Fork 0
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
Metric Webapp #9
Conversation
…ack|updated readme and setup components
Feature/project setup
Feature/fetch data
Feature/detail view
Implemented filter to only display air preassure in home
Updated ui|build files for deploy
Updated deployment url
Feature/general modifications
Feature/general modifications
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: CHANGES REQUIRED ♻️ ♻️
Hi @RileyManda,
Good job so far!
✔️ Nice use of color stripe in details page
✔️ Implementation of search functionality
However, there are some issues that you still need to work on to go to the next project but you are almost there!
Required Changes ♻️
Check the comments under the review.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear. And please, remember to tag me 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.
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.
README.md
Outdated
|
||
I would like to express my sincere gratitude to [Microverse](https://github.com/microverseinc), the dedicated reviewers, and collaborators. Your unwavering support, feedback, and collaborative efforts have played an immense role in making this journey a resounding success. I am truly grateful for your contributions and for being an integral part of my achievements. Thank you for your continued support. | ||
|
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.
- Your acknowledgment section should contain credits for Nelson Sakwa whose design was used for this project, in keeping with the Creative Commons License
src/components/Home.js
Outdated
<StyledHome> | ||
<TopCard | ||
backgroundImage={Map1} | ||
location="USA" | ||
views="890 Views" | ||
footerText="Stats by air preassure" | ||
toggleSearch={toggleSearch} | ||
isSearchVisible={isSearchVisible} | ||
setSearchKeyword={setSearchKeyword} | ||
/> |
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.
-
Kindly modify the intro section of your app to provide more relevant information about the project you are building. Adding an arbitrary 860 views from the USA does not fit the project you are building. You are concentrating on Air pressure, then kindly provide information related to it.
The intro section should provide more relevant information about the project, just like the design
src/components/Details.js
Outdated
<div> | ||
<TopCard | ||
backgroundImage={Map4} | ||
location={locationData.location} | ||
views="700 Views" | ||
footerText="CITY/TOWN BREAKDOWN-2013" | ||
setSearchKeyword={setSearchKeyword} | ||
toggleSearch={toggleSearch} | ||
isSearchVisible={isSearchVisible} | ||
/> |
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.
src/components/Home.js
Outdated
<Container fluid data-testid="content-card"> | ||
<Row xs={1} sm={2} md={2} lg={2} className="g-4 no-gutters p-2"> | ||
{filteredLocations.length > 0 ? ( | ||
filteredLocations.map((location, index) => { | ||
const measurementKey = uuidv4(); | ||
const truncatedTitle = location.location.length > 20 ? `${location.location.slice(0, 20)}...` : location.location; | ||
return ( | ||
<Col key={uuidv4()} xs={6} md={6}> | ||
<Link to={`/details/${location.location}/${uuidv4()}`} style={{ textDecoration: 'none' }}> | ||
<Card className={`content-card flex-container flex-column bold no-border flex-end ${index % 2 === 1 ? 'darker' : ''}`} style={{ backgroundImage: `url(${Map2})` }}> | ||
<Card.Header className="position-absolute top-0 end-0"> | ||
<FontAwesomeIcon icon={faCircleRight} style={{ color: '#fff' }} /> | ||
</Card.Header> | ||
<Card.Body | ||
style={{ display: 'flex', flexDirection: 'column', fluid: true }} | ||
className="card-content flex-container flex-column" | ||
> | ||
|
||
<div className="card-info-container flex-container flex-column"> | ||
<Card.Title style={{ width: '6rem' }} className="title-text white-text bold ellipsis- multiline-2 text-wrap"> | ||
{' '} | ||
{truncatedTitle.toUpperCase()} | ||
</Card.Title> | ||
</div> | ||
<div className="measurements-text white-text wrap-break"> | ||
{location.measurements.map((metric) => { | ||
if (airQualityParameters.includes(metric.parameter)) { | ||
return ( | ||
<div key={measurementKey} className="measurements-item"> | ||
{metric.parameter} | ||
{' '} | ||
: | ||
{' '} | ||
{metric.value} | ||
{' '} | ||
</div> | ||
); | ||
} | ||
return null; | ||
})} | ||
</div> | ||
|
||
</Card.Body> | ||
</Card> | ||
</Link> | ||
</Col> | ||
); | ||
}) |
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.
-
The design for the items on the Homepage should follow the alternating color patterns as we have on the design. All the darker colors are on one side, but the design requires that you alternate the darker and lighter colors. Kindly do that.
Your design without alternating color pattern Required Design with alternating color pattern
src/App.js
Outdated
<Header /> | ||
<Routes> | ||
<Route path="/" element={<Home />} /> | ||
<Route path="details/:location" element={<Details />}> |
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.
-
Each item on the home page should have a unique route. Right now, all your items have the same route:
/details
, but they should be something like so:/details/:id
. Kindly fix that.For example (you don't need to use the same word), instead of all your routes having the same route like so:
/details
They can have this:
/details/chipper-hill /details/san-antonio
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 the review and feedback @TedLivist
I will make sure to make the requested changes and submit for review. 🙏
Feature/general modifications
I have completed the task of implementing the suggested changes as requested by the reviewer. The following implementations were made: 🌱
In addition, I have ensured that this project fulfills the following default requirements : ✅ - There are no linter errors. Please review my implementation and suggest any improvements and changes. Thank you 🙏 |
Implemented chekcer pattern
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 @RileyManda 👋🏼👋🏼👋🏼,
Your project is complete! There is nothing else to say other than... it's time to merge it
Congratulations! 🎉
Highlights✅✅✅
- Acknnowledgement has been given to the author of the design as requested👍🏼.
- Relevant information has been provided on the intro page as requested👍🏼.
- The alternating color switch pattern has been implemented as requested👍🏼.
- The routing of the page has been properly implemented as requested👍🏼.
Optional suggestions
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, remember to tag me in your question 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.
I have completed the task of building the Metrics web app according to the stipulated requirements:
The following implementations were made: 🌱
In addition, I have ensured that this project fulfills the following default requirements :
✅ - There are no linter errors.
✅ - Used correct flow Gitflow.
✅ - Documented my work in a professional way.
✅ - Followed the list of the best practices for HTML & CSS and JavaScript.
Please review my implementation and suggest any improvements and changes.
Thank you 🙏