-
Notifications
You must be signed in to change notification settings - Fork 25
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
1574/remove business logic from from ListingsList component #1773
Conversation
✔️ Deploy Preview for dev-bloom ready! 🔨 Explore the source changes: 2e1947d 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/612966806d1a2f0008030ee7 😎 Browse the preview: https://deploy-preview-1773--dev-bloom.netlify.app |
32ddace
to
2e1947d
Compare
✔️ Deploy Preview for clever-edison-cd22c1 ready! 🔨 Explore the source changes: 78d22d1 🔍 Inspect the deploy log: https://app.netlify.com/sites/clever-edison-cd22c1/deploys/612965a260cff6000750df2c 😎 Browse the preview: https://deploy-preview-1773--clever-edison-cd22c1.netlify.app |
✔️ Deploy Preview for dev-partners-bloom ready! 🔨 Explore the source changes: 2e1947d 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/612966805203a50007c2a751 😎 Browse the preview: https://deploy-preview-1773--dev-partners-bloom.netlify.app |
✔️ Deploy Preview for dev-storybook-bloom ready! 🔨 Explore the source changes: 2e1947d 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/6129668052fc0200081af847 😎 Browse the preview: https://deploy-preview-1773--dev-storybook-bloom.netlify.app |
✔️ Deploy Preview for clever-edison-cd22c1 ready! 🔨 Explore the source changes: 2e1947d 🔍 Inspect the deploy log: https://app.netlify.com/sites/clever-edison-cd22c1/deploys/61296680e0b51400084e74ac 😎 Browse the preview: https://deploy-preview-1773--clever-edison-cd22c1.netlify.app |
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.
Looks good to me. The new functions, getListingImageCardStatus, getListingCardSubtitle and getListingTableData, look like good candidates for unit tests. If there's urgency to get this in, then can you please create an issue to write tests for these and add it to the backlog.
As it's blocking DAHLIA I'll merge and write the tests today 👍 |
Pull Request Template
Issue
Addresses #1574
Description
A request from SF to remove business logic from the ListingsList component by creating a new ListingCard component that takes in the image props and the table props.
Type of change
How Can This Be Tested/Reviewed?
Ensure the /listings page renders as expected
Checklist: