-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(Media): add component #94
Conversation
|
||
return ( | ||
<Tag {...attributes} className={classes}> | ||
{left} |
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.
This kind of goes against one of the goals for the project. "content is expected to be composed via props.children rather than using named props to pass in Components." Mostly for consistency. Any reason you didn't use a generic Media component for left/right. Placement could be determined by the order it is presented in the markup.
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.
Ok, I see. Would it look like this?
<Media>
<MediaLeft ... />
<MediaBody ... />
<Media>
<Media>
<MediaBody ... />
<MediaRight ... />
<Media>
I think if we have a generic component for left/right/body, it's difficult to distinguish children=[left,body] from children=[body,right].
<Media>
<MediaItem ... /> // left or body?
<MediaItem ... /> // body or right?
</Media>
Is this what you had in mind for generic Media for left/right, or something different?
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.
What about <Media><Media left/><MediaBody/></Media>
and <Media right/>
@eddywashere I updated the code to use I think the code would be more consistent with |
I think that makes sense |
@eddywashere I agree, I think it makes the usage of the media component easier to remember (more consistent). |
@eddywashere I updated the code. A couple of notes:
|
Changes Unknown when pulling 817d8b1 on mking-clari:media into * on reactstrap:master*. |
<Tag {...attributes} className={classes}> | ||
{React.Children.toArray(children).map((child, i) => { | ||
return React.cloneElement(child, { | ||
key: i, |
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.
Not sure about childTag
. Would it be better to push people to add <Media tag="li"/>
. This seems a little strict by cloning each element inside of MediaList.
@mking-clari thanks a ton for this pr and all the updates. I wish I would have had time to do the feedback all at once. I have that one last comment about |
@eddywashere thanks for the feedback, I think you've been really helpful and responsive, especially compared to many other github maintainers. I'm happy to contribute a small part to an active repository. The code should be updated with the latest change. Notes:
|
good call 👍 |
References #75
Notes
MediaHeading
andMediaObject
.MediaMarginal
for shared code betweenMediaLeft
andMediaRight
. It's not intended for direct use by users, but is exposed so we can test it.MediaList
, I'm applyingtag="li"
to the children. This way, the user ofMediaList
doesn't have to specifyli
on eachMedia
item./components/media
, then/components/buttons
, then hits back, the placeholder image isn't populated.