Skip to content
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

Remove minHeight prop #1265

Merged
merged 2 commits into from
Nov 21, 2024
Merged

Remove minHeight prop #1265

merged 2 commits into from
Nov 21, 2024

Conversation

dzole0311
Copy link
Collaborator

@dzole0311 dzole0311 commented Nov 20, 2024

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
  3. 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:

Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 4f9c679
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/673e12e39435940008700c40
😎 Deploy Preview https://deploy-preview-1265--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@slesaad
Copy link
Member

slesaad commented Nov 20, 2024

@dzole0311 isn't that prop needed for the default solid color fill in case an image isn't provided?

@dzole0311
Copy link
Collaborator Author

@slesaad I had to clean up my local cache (was switching between the Next.js repo and this one) before I noticed the problem 🤦 I pushed one more change, and would appreciate another look!

Also, I’ve updated our mock data to exclude the image for one of the cards which makes it easier to spot if something looks odd in next releases.

@dzole0311
Copy link
Collaborator Author

GHG Center preview: https://deploy-preview-667--ghg-demo.netlify.app/

Copy link
Member

@slesaad slesaad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks so much!

Copy link
Contributor

@aboydnw aboydnw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual testing looks good to me. Although I'm not sure the answer to @slesaad 's question:

isn't that prop needed for the default solid color fill in case an image isn't provided?

Did we confirm the default solid color fill still works as expected? I couldn't find an example in the preview link.

@slesaad
Copy link
Member

slesaad commented Nov 20, 2024

@aboydnw - @dzole0311 fixed that issue in the new commit, there's an example in this page https://deploy-preview-1265--veda-ui.netlify.app/stories

@dzole0311
Copy link
Collaborator Author

Thanks all for verifying the fix!

@aboydnw, as @slesaad mentioned, you can review the "External Link Test" card on this page (not the GHG Center preview). It has no media and displays the default background color.

@dzole0311 dzole0311 merged commit 14fc8ea into main Nov 21, 2024
9 checks passed
@dzole0311 dzole0311 deleted the 1264-card-images-fix branch November 21, 2024 07:46
dzole0311 added a commit that referenced this pull request Nov 26, 2024
### 🚀 Improvements
* Exploration & Analysis core VEDA feature breakout
#1251
#1154
* Introduce `EnvConfigProvider` to simplify the injection of environment
variables from host applications like Next.js
#1253
* Remove React Router's `useNavigate` dependency for simplifying the
routing #1270

### 🐛 Fixes
* Fix an issue where card images fail to maintain the correct aspect
ratio #1265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants