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

Tag component #434

Merged
merged 53 commits into from Aug 14, 2019

Conversation

@AquiGorka
Copy link
Member

commented Aug 2, 2019

image

@AquiGorka AquiGorka requested a review from bpierre Aug 2, 2019

@AquiGorka AquiGorka requested a review from dizzypaty Aug 2, 2019

@bpierre

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

🙌 🙌 🙌

I was imagining we would remove / deprecate Badge and its sub components at the same time we add Tag 🤔 What do you think?

@luisivan luisivan referenced this pull request Aug 2, 2019
31 of 55 tasks complete
@AquiGorka

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

@bpierre I fear this commit a526fe1 is as dangerous as deploying to production on a Friday.

@AquiGorka AquiGorka force-pushed the feature/tag branch 2 times, most recently from 34a7128 to 2f3eea5 Aug 2, 2019

@AquiGorka AquiGorka force-pushed the feature/tag branch from 2f3eea5 to 04f2223 Aug 2, 2019

@bpierre

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

@AquiGorka haha yes it would probably break a few apps in the current state 😄

I think we should ensure the previous APIs don’t break by redirecting some components, while recommending something else:

  • Badge => render & recommend Tag (already done).
  • Badge.Info => render & recommend Tag (later we’ll recommend Help).
  • Badge.Identity => render Tag, recommend IdentityBadge.
  • Badge.App => render & recommend Tag (later we’ll recommend AppBadge).
  • Badge.Notification => render & recommend Count.
  • BadgeNumber => render & recommend Count.

Could we also add a quick note to UPGRADE.md about this? Just to tell people that Badge is now Tag in most cases.

AquiGorka added some commits Aug 2, 2019

@sohkai
Copy link
Member

left a comment

Can we update src/Badge/README.md for the deprecation (none of the props are relevant anymore), add a new README for Tag, and add it back to the gallery?

Also, should we provide Count?

src/components/Tag/Tag.js Outdated Show resolved Hide resolved
src/components/Tag/Tag.js Outdated Show resolved Hide resolved
src/components/Tag/Tag.js Outdated Show resolved Hide resolved
UPGRADE.md Outdated Show resolved Hide resolved

bpierre and others added some commits Aug 8, 2019

@AquiGorka

This comment has been minimized.

Copy link
Member

commented on src/components/Tag/Tag.js in c3bda11 Aug 12, 2019

I sometimes get a console error when using this syntax, that's why I went with the ternary. Somewhere in the lines of ...css can not _something, something_ with boolean, if I see it I'll be sure to mention it.

This comment has been minimized.

Copy link
Member Author

replied Aug 12, 2019

I think we use this syntax elsewhere with css but do agree keeping it to a string is probably safer 👍

bpierre added some commits Aug 12, 2019

Theme: add colors related to the Tag component
Also removes tag, tagContent, indicator, indicatorContent, and
indicatorIcon.
Tag: final modes
- Implement the final modes.
- Add a `label` and `icon` props, to provide some spacing automatically.
- Move Count inside Tag (any mode can be a count so it makes more sense).
- Update the colors.
- Update the Badge compatibility components.
- Update the devbox demo.
- Update the documentation.
UPGRADE.md Outdated Show resolved Hide resolved
src/components/Tag/README.md Outdated Show resolved Hide resolved
src/components/Tag/README.md Outdated Show resolved Hide resolved
src/components/Tag/README.md Outdated Show resolved Hide resolved
src/components/Tag/Tag.js Outdated Show resolved Hide resolved
src/components/Tag/Tag.js Outdated Show resolved Hide resolved
src/components/Tag/Tag.js Outdated Show resolved Hide resolved

bpierre and others added some commits Aug 14, 2019

Update src/components/Tag/README.md
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>
@AquiGorka

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

I don't think <Tag count... /> is setting the correct padding for one char and it does not look like a circle:

image

@bpierre

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@AquiGorka Looks like a bug, on it!

bpierre added some commits Aug 14, 2019

@bpierre

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@AquiGorka It should be fixed now.

bpierre added some commits Aug 14, 2019

@bpierre bpierre merged commit c71b2d7 into newstyle Aug 14, 2019

5 checks passed

License Compliance All checks passed.
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@delete-merged-branch delete-merged-branch bot deleted the feature/tag branch Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.