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

refactor(Label): update props to the latest specs #474

Closed
levithomason opened this issue Sep 7, 2016 · 4 comments
Closed

refactor(Label): update props to the latest specs #474

levithomason opened this issue Sep 7, 2016 · 4 comments

Comments

@levithomason
Copy link
Member

levithomason commented Sep 7, 2016

Shorthand

Currently, the Label allows shorthand props icon, image, and text along with children. These should be disallow()ed via propTypes and early returns put in place to render one or the other.

text

This prop should be renamed to content. See #391.

link

This prop renders as an a tag. It should go away as we now have augmentation <Label as='a' /> instead.

empty

Currently, the Label auto applies the empty className when there are no children. Magic is bad, unless it is magic application of something that is always required, like a Checkbox fitted prop. A Label with no children does not always require the empty class. It is a slightly larger but valid Label without it:

image

We should remove the auto application of this className.

Docs & Tests

Docs and tests should then be updated for all usages.

@jeffcarbs
Copy link
Member

Somewhat related - I'm having an issue with this line which adds the empty class: https://github.com/TechnologyAdvice/stardust/blob/master/src/elements/Label/Label.js#L41.

I'm trying to set a circular label with the text shorthand prop and it's getting the empty class. This is because that logic is only based on children:

// Currently
<Label text={count} circular />
<div class="ui empty circular label">11</div>

// To make it non-empty, needs children
<Label circular>{count}</Label>
<div class="ui circular label">11</div>

I could see empty being set when there's no content of any kind, although I think I'd prefer to just set it manually:

// Better (but still not completely intuitive):
<Label circular />
<div class="ui empty circular label"></div>

// Ideal:
<Label empty circular />
<div class="ui empty circular label"></div>

@levithomason
Copy link
Member Author

@levithomason
Copy link
Member Author

Actually, let's just do this all at once, updated description above.

@levithomason
Copy link
Member Author

Fixed in #486

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants