BREAKING: Chips redesign #369
Conversation
|
Hey @gawrysiak, thank you for your pull request 🤗. The documentation from this branch can be viewed here. |
satya164
force-pushed
the
material-next
branch
2 times, most recently
from
5613de2 to
c970557
Compare
satya164
left a comment
satya164
left a comment
There was a problem hiding this comment.
Thanks for the PR! Can you rebase and drop the unrelated commits?
src/components/Chip.js
Outdated
| /** | ||
| * Displays the chip as pressed. | ||
| */ | ||
| pressed?: boolean, |
There was a problem hiding this comment.
Instead of pressed prop, we should automatically apply the styles on onPressIn and remove them in onPressOut (only when onPress prop is provided).
There was a problem hiding this comment.
Also, the pressed style has a elevation shadow (similar to button)
src/components/Chip.js
Outdated
| /** | ||
| * Displays chip with outline. | ||
| */ | ||
| outlined?: boolean, |
There was a problem hiding this comment.
Instead of outlined prop, can you add a mode prop similar to button? The values can be flat and outlined, and default would be flat.
There was a problem hiding this comment.
yeah, I was looking into the button. Couldn't figure out the default mode name. Flat is good 👍
src/components/Chip.js
Outdated
| ]} | ||
| > | ||
| {icon || selected ? ( | ||
| /* $FlowFixMe */ <Icon |
There was a problem hiding this comment.
Icon margin doesn't seem right:
should look something like:
Should be same as close button padding, which should be reduced a bit:
src/components/Chip.js
Outdated
| size={20} | ||
| /> | ||
| ) : null} | ||
| {avatar |
There was a problem hiding this comment.
Probably shouldn't allow both icon and avatarto be specified
There was a problem hiding this comment.
For selected state with avatar, a check mark should appear as overlay on the avatar
gawrysiak
force-pushed
the
breaking/chips-redesign
branch
3 times, most recently
from
a56adc8 to
af160cc
Compare
|
Updated:
|
gawrysiak
force-pushed
the
breaking/chips-redesign
branch
from
5b68f61 to
f6d1b56
Compare
satya164
force-pushed
the
material-next
branch
2 times, most recently
from
f394776 to
430b3e6
Compare
gawrysiak
force-pushed
the
breaking/chips-redesign
branch
2 times, most recently
from
ef4b054 to
8344078
Compare
satya164
force-pushed
the
material-next
branch
3 times, most recently
from
cf0531b to
1d511cc
Compare
gawrysiak
force-pushed
the
breaking/chips-redesign
branch
from
8344078 to
59f52ae
Compare
satya164
force-pushed
the
material-next
branch
14 times, most recently
from
ef02cd7 to
d8182b4
Compare
|
Selected with image looks like it has a padding issue on the left due to longer text? |
|
@8BallBomBom you're right, nice catch. It has been fixed in a later commit (59f52ae). |
|
Hi @gawrysiak, could you suggest how to style the actived state of Chip (with difference text color), similar this screenshot - https://user-images.githubusercontent.com/1174278/39865846-895aaf44-544e-11e8-9097-95dd3d67a525.png Thank you! |
5 participants
Chip redesign proposal for 2.0.
Motivation
Fixes #357.
Test plan
Snapshot tests updated. Run
yarn test.