-
Notifications
You must be signed in to change notification settings - Fork 23
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
Kds 1755/spot illustration migration #4208
Conversation
🦋 Changeset detectedLatest commit: f4ab31a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
||
When not enabled, the `img` tag will attempt to fill a width of 100%. | ||
|
||
## All Spot illustrations |
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.
I've included the stickersheet that has a list of the illustrations since our consumers would not be able to see the hidden stories on prod.
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.
I think just to reiterate, put an example of the import statement before the canvas
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.
Good idea, I'll make a quick update to include that example
const SPOT_ILLUSTRATION_BASE_PATH = "illustrations/heart/spot/" | ||
const createSpotIllustration = | ||
(fileName: string) => | ||
// eslint-disable-next-line react/display-name |
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.
I feel like there might be a better way here to give these display names instead of just JSX elements, but I decided not to fall down the refactor rabbit hole - best this could be looked at when we revisit the names of these
const engagementSpots = [ | ||
{ | ||
Component: BenefitsSurvey, | ||
name: "BenefitsSurvey", |
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.
Originally these had more human readable names, but that doesn't help our consumer know what they will actually be exported as. I was thinking we wouldn't need this and could just use the displayName but that's not actually set for these components so I just went with the more verbose approach of remove the spaces and making sure they're in pascal case / align with the component name
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.
Just check your chromatic snapshots
Why
Part of the Great Migration
What
Positive
has existed for a while now an is a suitable alternative for eitherNotes