Skip to content
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(component): Add chip component #189

Merged
merged 4 commits into from
Sep 5, 2019
Merged

feat(component): Add chip component #189

merged 4 commits into from
Sep 5, 2019

Conversation

animesh1987
Copy link
Contributor

@animesh1987 animesh1987 commented Sep 5, 2019

What

  • Add chip component.

Testing

  • Jest tests

@jorgemoya

(cherry picked from commit de4c480fb1dee5a6170dde3bde832fd3a5f9844d)
@deini deini requested a review from a team September 5, 2019 15:23
@deini
Copy link
Member

deini commented Sep 5, 2019

Hey @animesh1987 👋 pushed a commit with a couple of changes mainly:

  • Changed the API to <Chip>Text</Chip>
  • There is only one prop now, onDelete which we also use to display/hide the close button.
  • The close svg is now wrapped in a Button component for accessibility

></StyledCloseButton>
);

return (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even want to render the chip if there is no label?

Suggested change
return (
return label ? (

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add the above, you should add another test with no children.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that but I think it's a bit easier to spot if we render a chip which looks wrong than to not render at all. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just from my own experience experimenting with chips, I think it's better if we display an empty chip than no chip at all. I was able to debug I wasn't sending the right text immediately.


{renderDeleteButton()}
</StyledChip>
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addition from above.

Suggested change
);
) : null;

export const StyledChip = styled(Box)`
align-items: center;
display: inline-flex;
user-select: none;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍹Is there a specific reason why we wouldn't want people to select the text?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, it just looks terrible and doesn't seem that useful 🤔

></StyledCloseButton>
);

return (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just from my own experience experimenting with chips, I think it's better if we display an empty chip than no chip at all. I was able to debug I wasn't sending the right text immediately.

@deini deini merged commit 8258f5e into bigcommerce:master Sep 5, 2019
@animesh1987
Copy link
Contributor Author

Thanks for pushing it through @deini 👍

@animesh1987 animesh1987 deleted the chip-component branch September 5, 2019 23:57
amckemie pushed a commit to amckemie/big-design that referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants