-
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
NW6| Rabia Avci | React Module Project | Feature Deck - Week1 #40
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.
Looking great 👍 Added a note worth reading up on if you have time, but it's not really an issue as such, more of a consideration.
return ( | ||
<div className="deck"> | ||
{cardsData.map((cardData, i) => ( | ||
<Card title={cardData.title} url={cardData.url} image={cardData.image} key={i} /> |
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.
Good work with the props :)
This doesn't really matter in this situation (it would only really matter when you're dealing with large amounts of components), but the key shouldn't really be an index. Have a read through the section here under "Keeping List Items in Order with Key" https://react.dev/learn/rendering-lists
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.
yes, id would be better solution 👍 Thank you!
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.
Well done @RbAvci
const cardElement = cards[index]; | ||
expect(cardElement).toHaveTextContent(cardData.title); | ||
expect(cardElement.querySelector("a")).toHaveAttribute("href", cardData.url); | ||
expect(cardElement.querySelector("img")).toHaveAttribute("src", cardData.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.
Good work 👏
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 liked your work.
return ( | ||
<div className="deck"> | ||
{cardsData.map((cardData, i) => ( | ||
<Card title={cardData.title} url={cardData.url} image={cardData.image} key={i} /> |
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.
Well done @RbAvci
✅ Deploy Preview for cyf-react-hotel-project ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Learners, PR Template
Self checklist
Changelist
Completed Deck component with 3 cards component(which data comes from fakeCards.json). Added tests for Deck feature.
I would be very happy if you review my code, comment and approve it.