[WIP] feat(vital-card): JEPZ-408 | Hero Card 2.0 #2211
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
Thank you for creating a pull request on GitHub! We appreciate your contribution to our repository. In order to enable our tests to run on your changes, we kindly ask that you wait until one of our repository members adds the ready-to-test label to your pull request. (Please request on the GitHub issue you are addressing.) This will ensure that our testing process runs smoothly and we can quickly verify the changes you've made. Thank you for your understanding and we look forward to reviewing your changes soon! |
| * Tertiary Hero Card Button. | ||
| * Plain vital button with `WithTertiaryStyle` token and `READ MORE` placeholder. | ||
| */ | ||
| const TertiaryHeroCardButton = asButtonToken( |
There was a problem hiding this comment.
I'd probably combine these cards, they have a lot in common. Something like this:
const PlainButton = asButtonToken(vitalButton.Plain, {
Content: {
Body: withPlaceholder('BUTTON'),
},
});
// And then
const PrimaryHeroCardButton = asButtonToken(PlainButton, vitalButton.WithPrimaryStyle),
const SecondaryHeroCardButton = asButtonToken(PlainButton, vitalButton.WithSecondaryStyle),
// ...| ImageWrapper: 'px-16px md:px-0px', | ||
| }, | ||
| }); | ||
| const HorizontalWithImageLeft = asHeroCardToken( |
There was a problem hiding this comment.
I think these orientational tokens should also be named with WithHorizontalWithImageLeft and With HorizontalWithImageRight.
I'd also keep the names consistent. We have WithHorizontalRightOrientation exported from base. I thought keeping the names of these tokens may help us in future. What I did for some other cards is changing the names of imports like
import Base, {
WithHorizontalLeftOrientation as WithHorizontalLeftOrientationBase,
WithHorizontalRightOrientation as WithHorizontalRightOrientationBase,
// ...
} from '../../Card/tokens/Base';and then in this file define WithHorizontalLeftOrientation and WithHorizontalRightOrientation that will be part of the token collection.
This way we would have consistent token names across multiple cards
| } | ||
| }); | ||
|
|
||
| const WithSecondaryButton = asHeroCardToken({ |
There was a problem hiding this comment.
We could probably consolidate these as well:
const WithButton = asHeroCardToken({
Components: {
CTALink: on(ButtonClean)(vitalButton.Plain),
},
Content: {
CTAText: withPlaceholder('Button Text'),
},
});
const WithPrimaryButton = asHeroCardToken(WithButton, {
Theme: {
CTALink: vitalButton.WithPrimaryStyle,
}
});
const WithSecondaryButton = asHeroCardToken(WithButton, {
Theme: {
CTALink: vitalButton.WithSecondaryStyle,
}
});I think this way you could also remove PrimaryHeroCardButton, SecondaryHeroCardButton and TertiaryHeroCardButton above.
| } | ||
| }); | ||
|
|
||
| const WithNoSubtitle = asHeroCardToken({ |
There was a problem hiding this comment.
I think we were trying to avoid WithNo... tokens. cc @hvanyo . Why would we have both WithSubtitle and WithNoSubtitle tokens.
There was a problem hiding this comment.
I forgot that this slot isn't part of the component by default when I was quickly writing all of the WithNo... tokens.
I'll remove this one.
However, the AC does ask for a token to remove both the description and image (both default slots).
Changes
Test Instructions
Related Issues