-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Feature/list examples #100
Conversation
</List> | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only did one example for the basic list - Semantic has a handful, would adding some variations be useful or necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. It looks like they just show a mashup of various lists at the top, then dive into each specific capability. I think we're good with the basics up top if we have links/icons/etc down below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍
description='This text will always have a left margin so it sits alongside the icon' | ||
/> | ||
<ListItem | ||
image={triangleIcon} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use icon props here, I won't comment on further icon/image props.
04d55e2
to
065dd73
Compare
Since I rebased this guy and updated it, would you check it out and make sure it still looks sane. If so, it is good to merge! 🉑 |
<div className={classes}> | ||
<img {...this.props} /> | ||
</div> | ||
<img {...this.props} className={classes} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you are returning this one element, the parentheses are not necessary and you can move the image up to line 26
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, will update and merge.
1 question, 1 comment, overall good to merge!! 🐙 |
No description provided.