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 #486

Merged
merged 1 commit into from
Sep 9, 2016

Conversation

jeffcarbs
Copy link
Member

@jeffcarbs jeffcarbs commented Sep 9, 2016

Addresses #474

Some comments/questions:

  • The issue description mentioned removing the link prop which set the element type to a but there was also logic so if image or onClick was passed it would be rendered as a. I removed that whole thing in favor of just using the as prop. I figured the less magic the better.
  • There's similar logic for the detail here. Thoughts on removing that logic and the detailLink prop in favor of a detailAs which would behave like as?

@levithomason
Copy link
Member

I would suspect users would want pointer cursors (a tags) when onClick is provided, however, I'll hardly fight for keeping magic :P

+1 to detailAs updates.

@jeffcarbs jeffcarbs force-pushed the feature/label-cleanup branch from f7604c9 to e363ffa Compare September 9, 2016 20:52
@codecov-io
Copy link

codecov-io commented Sep 9, 2016

Current coverage is 98.63% (diff: 100%)

Merging #486 into master will decrease coverage by <.01%

@@             master       #486   diff @@
==========================================
  Files           101        101          
  Lines          1467       1465     -2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           1447       1445     -2   
  Misses           20         20          
  Partials          0          0          

Powered by Codecov. Last update ad330a3...e363ffa

@jeffcarbs
Copy link
Member Author

Made the detail changes and rebased onto the latest master 👍

const ElementType = getElementType(Label, props, () => {
if (image || link || onClick) return 'a'
})
const DetailComponent = detailAs || 'div'
Copy link
Member

Choose a reason for hiding this comment

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

Hm. I think here we should be using getElementType() for consistent handling. This makes me realize that this component is actually missing sub components. It should have Label.Detail for instance. That would also solve all the issues related to it.

If you like, we can merge this as is, and open an issue to create Label sub components.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 for merging this as-is and opening a new issue to talk about sub components.

Quick thoughts:
There are a few sub components, but only detail can be semi-customized. The others are either there or not and are already covered by factories.

I think Label.Detail might make sense to break out into its own component with props:

  • as
  • children
  • content
  • onClick

It'd be really nice to keep a shorthand prop on Label for a vanilla detail, though. Can't think of an analogous component relationship off the top of my head.

Copy link
Member

Choose a reason for hiding this comment

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

@levithomason
Copy link
Member

Cool, will merge on pass. LMK what you want to do regarding sub components mentioned here

@levithomason levithomason merged commit df33805 into Semantic-Org:master Sep 9, 2016
@levithomason
Copy link
Member

Released in stardust@0.43.0, thanks!

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

Successfully merging this pull request may close these issues.

3 participants