-
Notifications
You must be signed in to change notification settings - Fork 6
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
DSD-1520: Add sizeBasedOn prop to Logo #1456
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
The code looks good. I was trying to see if the logic in logo.ts
could be done so that there's one object since the pixel values are the same for both size
and sizeBasedOn
. But the maxWidth and maxHeight does make that a bit difficult, and also the largest two sizes.
One thing that this is missing is the new component changelog table. Here's an example implementation: https://github.com/NYPL/nypl-design-system/blob/development/src/components/Slider/Slider.mdx#L339
and an example data file: https://github.com/NYPL/nypl-design-system/blob/development/src/components/Slider/sliderChangelogData.ts
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! Thanks for the last update. Let's also wait for Marty to review because he knows the styles better than I do.
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.
Works great! Just make sure to update the TOC on the story page.
Fixes JIRA ticket 1520
This PR does the following:
How has this been tested?
Locally in Storybook
Accessibility concerns or updates
Checklist:
Front End Review: