Card image/description independent of hero image/description #1244
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Yay, prettier formatting 🙌
Just leaving some thoughts I had when reading the code.
Testing the branch locally works as stated, thank you for listing the detailed verification steps!
Do you think we can consolidate the logic of when to display what in some testable functions, and actually add tests for them? Maybe unit tests, or even via playwright? This way we have a good documentation of the indented behavior as well, for future reference.
| cardDescription: '' | ||
| } as DatasetData; | ||
|
|
||
| expect(getDescription(data)).toBe(''); |
There was a problem hiding this comment.
Thats an interesting one! Do we want the card title to be blank in that case, or should we treat an empty string as null and display the fallback?
Just wondering, we can discuss it once we hit the issue! Glad we have the test specifying the current behavior 🙏
There was a problem hiding this comment.
I wanted to test how the markdown parser handles empty strings, but in terms of fallbacks, there's no fallback for the description e.g. we will not show anything if description or cardDescription are empty or missing (so the test reflects the current behavior)
| imgSrc={cardMedia?.src} | ||
| imgAlt={cardMedia?.alt} | ||
| description={getDescription(d)} | ||
| imgSrc={getMediaProperty(undefined, d, 'src')} |
There was a problem hiding this comment.
I'd prefer null instead of undefined in this case, to mark it as intentional.
We don't have to change it now, since we use undefined in several places throughout the code already, but maybe consider it for the future?
There was a problem hiding this comment.
Noted, will consider switching to null next time 👍
## 🎉 Features * Card image/description independent of hero image/description by @dzole0311 in #1244 ## 🚀 Improvements * Cookie consent code cleanup by @snmln in #1199 , and @hanbyul-here in #1240 , and @hanbyul-here in #1241 * Add ADR about design system change by @j08lue in #890 * Update condition to run playwright tests on release branches by @dzole0311 in #1228 * Update STYLE_GUIDE.md by @AliceR in #1227 * Fix lint configuration by @AliceR in #1219 * Add tests for the AOI feature specification by @AliceR in #1216 * Set data catalog filters to be closed by default by @vgeorge in #1243 * Update tsconfig and make nav interfaces exposable for consumption by @sandrahoang686 in #1223 ## 🐛 Fixes * Hotfix to hide the external link badge from cards by @dzole0311 in #1231 ## New Contributors * @vgeorge made their first contribution in #1243 **Full Changelog**: v5.9.0...v5.10.0
**Related Ticket:** #1264 ### Description of Changes Removed minHeight prop to avoid undesired card behavior. The css prop was introduced here: #1244 ### Notes & Questions About Changes _{Add additonal notes and outstanding questions here related to changes in this pull request}_ ### Validation / Testing 1. Open the stories page where there are card components 2. Verify that the images in the cards no longer have a minHeight property 4. Verify that the card layout appears consistent and the image heights are reduced compared to the previous behavior on v10.0.0 Or see the Air Quality card here: - Before: https://deploy-preview-1250--veda-ui.netlify.app/stories - After: https://deploy-preview-1265--veda-ui.netlify.app/stories
Related Ticket: US-GHG-Center/veda-config-ghg#636
Description of Changes
cardDescriptionandcardMediaprops to enable customizable card contentThese updates are applied across various places where the card component is used (story cards) but also layer cards in the Data Catalog and the E&A Catalog to better control displayed descriptions and images.
Notes & Questions About Changes
Validation / Testing
cardDescriptionto the 'This is the life of water' story in the MDX configuration file/storiesdisplays the newly addedcardDescriptionon the cardcardMediacardMediaand themediaprop -> verify that there's a fallback background color shownVerify that the behavior below (copied from the issues' acceptance criteria) is applied: