-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: use proper states and make UtilityCard more generic #1777
Conversation
🦋 Changeset detectedLatest commit: dec79d9 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 |
Size Change: +23 B (+0.05%) Total Size: 43.6 kB
ℹ️ View Unchanged
|
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 to me. Might be worthwhile to add a Story template. What do you think?
@@ -32,7 +33,11 @@ Default.args = { | |||
export const CardWithBadge: Story = Template.bind({}); |
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.
Maybe we can add CardWithContent here as well?
acc83d8
to
fbd64a5
Compare
fbd64a5
to
dec79d9
Compare
- Renamed `categoryTag` to `subTitle` to make it more generic | ||
- `onClick` prop is only required when using `clickable` as true | ||
|
||
Minor: |
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.
is it fine that this is a major update but we include minor updates?
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.
From what we have here, I would say it's fine. Do you see any specific issue with it?
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 to me
Description of the change
Breaking Change:
interactiveElementType
prop in order to add two new states propertieshoverable
andclickable
categoryTag
tosubTitle
to make it more genericonClick
prop is only required when usingclickable
as trueMinor:
hoverable
andclickable
to have more control of state of the cardemojiWrapperSize
prop with optionssmall
andlarge
so it's adaptable to different use casesindicator
to add a helper element to the top right of the cardTesting the change
Type of change
Development
Code review