-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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: add card mode outlined #2512
Conversation
Hey @brunohkbx, thank you for your pull request 🤗. The documentation from this branch can be viewed here. |
The mobile version of example app from this branch is ready! You can see it here |
* - `elevated` - Card with elevation, elevation defaults to 1. | ||
* - `outlined` - Card with an outline. | ||
*/ | ||
mode?: 'elevated' | 'outlined'; |
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 don't like the way it's typed currently. The outlined card shouldn't accept elevation prop. Could you try something like that:
type OutlinedCardProps = {
mode: 'outlined';
elevation: never;
};
type ElevatedCardProps = {
mode?: 'elevated';
elevation?: number;
};
type Props = (OutlinedCardProps | ElevatedCardProps) & { Common Props };
I haven't tried that, so it might not work, but I would like to explore this way of handling conditional types.
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.
@Trancever I tried your approach and it wasn't working at first. So after some time debugging, I found a problem in the HOC withTheme
. I removed it and it worked as follow:
Raises error
<Card mode="outlined" elevation={3} />
Works
<Card mode="elevated" elevation={3} />
After looking up the issue in withTheme
I found a problem in its type declaration here
$Without
it's not working as expected because Pick
doesn't distribute over discriminated union types and you can read more about here
Should we fix the type declaration first?
type $Without<T, K extends keyof any> = T extends any ? Pick<T, Exclude<keyof T, K>> : never;
works for me
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 we should first fix the type of the withTheme HOC. Could you submit a PR? 🙏
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.
1aeb869
to
e4f399d
Compare
Hello 👋, this pull request has been open for more than 2 months with no activity on it. If you think this is still necessary with the latest version, please comment and ping a maintainer to get this reviewed, otherwise it will be closed automatically in 7 days. |
e4f399d
to
1277a1b
Compare
1277a1b
to
24a755b
Compare
@lukewalczak I added a switch at the top to toggle the mode. WDYT? |
Hey @brunohkbx, thanks for updating the PR! I like the idea of switch and the whole feature 🎉 . I will test it a bit, ping the theme provider lib one more time and go through the code. Anyway I guess we're close to ship it! |
@brunohkbx I just published a new version of theme-provider with your fix. Sorry for the lag. |
@souhe thanks. I will update this PR |
24a755b
to
281f6e5
Compare
TS now complains when using mode |
Isn't a correct behavior since you typed:
? |
@lukewalczak yeah it is. I'm just showing it. Works now with |
Ahhh ok I see, sorry for misunderstanding 😅 thanks @brunohkbx! |
Awesome feature, thanks @brunohkbx for your work! |
Summary
Implement MD outlined card.
https://material.io/components/cards
Resolves #2130
Test plan