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

Components: Fix the icon button level prop #444

Merged
merged 5 commits into from
Apr 18, 2017
Merged

Conversation

youknowriad
Copy link
Contributor

This PR introduces a level prop for the IconButton instead of using the children prop for this purpose. Children can contain components etc... I think it's not suited for this.

Also, in #429 I'm using the children prop to display text next to the icon which is more appropriate IMO.

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 18, 2017
@youknowriad youknowriad self-assigned this Apr 18, 2017
@youknowriad youknowriad requested review from jasmussen and aduth April 18, 2017 13:25
@jasmussen
Copy link
Contributor

Took the liberty of tweaking the right margin of the number. I didn't notice it previously but it was slightly off. This looks better. Nice work! 👍

@aduth
Copy link
Member

aduth commented Apr 18, 2017

My original thinking was that since IconButton accepts arbitrary props to apply to the element, the data-level prop could be passed at the toolbar render than accepted as a level prop explicitly in the IconButton component itself.

@youknowriad
Copy link
Contributor Author

@aduth I'm not sure I understand? Could you clarify?

@aduth
Copy link
Member

aduth commented Apr 18, 2017

level is not a natural concept of a generic IconButton component. Instead of rendering:

<IconButton level="1" />

And the implementation of IconButton understanding the incoming level prop, we could do:

<IconButton data-level="1" />

And IconButton doesn't need to know about that prop, but will pass it down through to the rendered button anyways because of the treatment of the additionalProps spread.

@youknowriad
Copy link
Contributor Author

Tks for the explanation @aduth you raise a good point and it should be addressed now.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -30,6 +30,16 @@
background-color: $dark-gray-500;
color: $white;
}

&:after {
Copy link
Member

Choose a reason for hiding this comment

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

Likely no difference since styles ensure there's no impact on layout when empty, but this should probably only be applied if the control actually has a data-level attribute:

&[data-level]:after {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants