-
Notifications
You must be signed in to change notification settings - Fork 311
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): create media object component #1383
Conversation
Verified that @alexkrolick has signed the CLA. Thanks for the pull request! |
2260f42
to
b5e18e7
Compare
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.
Approach looks good to me!
src/components/media/Media.scss
Outdated
} | ||
|
||
// the media content, ie, an avatar | ||
.bdl-Media-img { |
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.
Each sub-component should have its own top-level class name, such as .bdl-MediaImage
. It's probably fine for them to live in the same stylesheet, though, as long as re-importing it in each component doesn't create duplicate styles in the output CSS.
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.
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.
That applies when different elements live in the same file, but since these are declared as separate components, they should be named as such.
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.
The sub-elements are meant to be used like Media.Body
not as separate imports.
These are sub-elements almost exactly like the Tweet example in the SUIT docs.
9daa060
to
edd1740
Compare
Probably should check if https://github.com/box/box-ui-elements/blob/master/src/elements/content-sidebar/versions/VersionsItem.js can be refactored to use these components/design if possible |
7c0cd5b
to
5acbafe
Compare
I tested this out with the Activity Feed Comment/Task container and it seems to work. I'll put that out as a separate PR once this is in. |
31c20aa
to
ebb54ba
Compare
83448e6
to
81162dd
Compare
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 job!
Thanks for your feedback everyone |
No description provided.