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.

Sign up for GitHub

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

Jump to bottom

DSD-1521: Add sizeBasedOn prop to Image #1465

Merged
merged 10 commits into from
Dec 6, 2023
Merged

DSD-1521: Add sizeBasedOn prop to Image #1465

merged 10 commits into from
Dec 6, 2023

Conversation

jackiequach
Copy link
Collaborator

@jackiequach jackiequach commented Nov 21, 2023 •
edited
Loading

Fixes JIRA ticket DSD-1521

This PR does the following:

  • Adds sizeBasedOn prop to Image component

How has this been tested?

Locally on Storybook

Accessibility concerns or updates

N/A

Checklist:

  • I have updated the Storybook documentation accordingly.
  • I have added relevant accessibility documentation for this pull request.
  • All new and existing tests passed.

Front End Review:

  • Review the Vercel preview deployment once it is ready.

@vercel Vercel
Copy link

vercel bot commented Nov 21, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nypl-design-system ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 6, 2023 2:57pm

@jackiequach jackiequach changed the title [WIP] DSD-1521: Add sizeBasedOn prop to Image DSD-1521: Add sizeBasedOn prop to Image Nov 30, 2023
@jackiequach jackiequach marked this pull request as ready for review November 30, 2023 16:15
EdwinGuzman
EdwinGuzman reviewed Nov 30, 2023
View reviewed changes
Copy link
Member

@EdwinGuzman EdwinGuzman left a comment

Choose a reason for hiding this comment

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

This looks good but I'm not sure if it's working when sizeBasedOn is set to height and the aspect ratios are updated. Or rather, is it only supposed to work when aspectRatio is original?

src/components/Image/imageChangelogData.ts Outdated Show resolved Hide resolved
src/components/Image/Image.tsx Outdated
@@ -29,9 +29,11 @@ export const imageSizesArray = [
"large",
] as const;
export const imageTypesArray = ["default", "circle"] as const;
export const imageSizeBasedOnArray = ["height", "width"] as const;
Copy link
Member

Choose a reason for hiding this comment

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

You can try to reuse the logoSizeBasedOnArray but rename it to something more generic for image and logo and any other component that may use height and width as values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added dimensionTypeArray to types.ts and changed the imports.

src/components/Image/Image.stories.tsx Outdated Show resolved Hide resolved
src/theme/components/image.ts Outdated Show resolved Hide resolved
src/theme/components/image.ts
height: "auto",
overflow: "hidden",
const sizes = {
xxxsmall: 32,
Copy link
Member

Choose a reason for hiding this comment

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

Do you not need the specific "px" text in the value? Don't think you need to change anything but I wonder if the number 64 is the same as the string "64px" in css.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I now see how you're using this. Never mind the comment!

src/components/Image/Image.mdx Show resolved Hide resolved
@jackiequach
Copy link
Collaborator Author

I tested sizeBasedOn height with different aspect ratio on the Storybook With Controls page and it works correctly. Let me know if I am misunderstanding.

@EdwinGuzman
Copy link
Member

The update looks good, thanks.

This is what I mean regarding the aspect ratio but I may also not understand the reqs. This is a somewhat exaggerated example. For sizedBasedOn="height" and src="//placekitten.com/400/40", when aspectRatio="original" I get this which seems right:
Screen Shot 2023-12-01 at 2 31 56 PM

If I keep the src the same with the height of 40px but update the aspect ratio, it's no longer 40px. This is what I'm unsure of. If the aspect ratio is updated to "square", I get the following:

Screen Shot 2023-12-01 at 2 33 51 PM

@jackiequach
Copy link
Collaborator Author

If I keep the src the same with the height of 40px but update the aspect ratio, it's no longer 40px. This is what I'm unsure of. If the aspect ratio is updated to "square", I get the following:

Screen Shot 2023-12-01 at 2 33 51 PM

This matches the behavior of size based on width. The height of the image will be determined by the size prop.

@EdwinGuzman
Copy link
Member

Hmm yeah that makes more sense. Let's wait on Marty to review this but looks good otherwise.

@bigfishdesign13
Copy link
Collaborator

Overall, this is working well, but I am seeing one issue. When sizeBasedOn is set to "height" and the height of the actual image is less than the height set by the size value, the image does not scale up to match the size height value. It doesn't go any larger than the actual image height. A larger image will scale down to match the size height value, but a smaller image is not being scaled up. Conversely, when sizeBasedOn is set to "width", a small image will scale to match the size width value.

I think this is what @EdwinGuzman was also seeing.

bigfishdesign13
bigfishdesign13 approved these changes Dec 5, 2023
View reviewed changes
Copy link
Collaborator

@bigfishdesign13 bigfishdesign13 left a comment

Choose a reason for hiding this comment

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

Very nice. The last update works great.

@bigfishdesign13 bigfishdesign13 added the Ship It Pull requests that have been reviewed and approved. label Dec 5, 2023
@jackiequach jackiequach merged commit 7b3ae13 into development Dec 6, 2023
5 checks passed
@jackiequach jackiequach deleted the DSD-1521/image-sizeBasedOn branch December 6, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bigfishdesign13 bigfishdesign13 bigfishdesign13 approved these changes

@EdwinGuzman EdwinGuzman EdwinGuzman approved these changes

Assignees
No one assigned
Labels
Ship It Pull requests that have been reviewed and approved.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jackiequach @EdwinGuzman @bigfishdesign13