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

[CardMedia] Add component property #8376

Merged
merged 1 commit into from Sep 28, 2017

Conversation

AndriusBil
Copy link
Contributor

Fixes: #8233

If component="img" specified, then renders img element instead of div.
Now it's possible to set srcSet and sizes attributes.
Backwards compatible (in case any users already have custom styling for div e.g. css image-set)

No documentation / examples as of initial PR.
If this implementation is acceptable, I'll add some documentation.

ComponentProp === 'div' ? { backgroundImage: `url(${image})`, ...style } : style;
const composedClassName = classNames(
{
[classes.root]: ComponentProp === 'div',
Copy link
Member

Choose a reason for hiding this comment

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

What if component will be neither div nor img? I think in this case it should receive classes.root styles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for review!

That's a proper point. I think backgroundImage should also be added to style prop in case anything else than img is specified.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for contribution ;)

@oliviertassinari oliviertassinari added component: card This is the name of the generic UI component, not the React module! v1 labels Sep 25, 2017
@oliviertassinari oliviertassinari changed the title [CardMedia] Rendering component from component prop added. [CardMedia] Add component property Sep 25, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 25, 2017

@AndriusBil Sounds good. The only thing that feels weird is using the image property with a img component. It would feel more intuitive using the src property.

@oliviertassinari oliviertassinari added the new feature New feature or request label Sep 25, 2017
@AndriusBil
Copy link
Contributor Author

Thanks for comment!

I think it also feels a bit weird using src with div or table or probably most of other tags. Should I add src as alias to image or should I remove image prop?

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 26, 2017

Should I add src as alias to image

@AndriusBil I haven't been thinking about this solution. This one sounds good 👍 .

@AndriusBil
Copy link
Contributor Author

I've added src prop as alias for image prop. src is only used with media components (video, audio, picture, iframe, img). For other components image is added as a css background.

While implementing it, I started questioning what is the reason for this component?

If we actually wanted to display media (be it image or video or something else) we could use standard html media components. CardMedia only adds width: 100% style for it.

@oliviertassinari oliviertassinari self-assigned this Sep 27, 2017
@oliviertassinari
Copy link
Member

@AndriusBil Well done :)

@adisher
Copy link

adisher commented Jul 26, 2020

CardMedia only adds width: 100% style for it.
This line alone helped me to understand my issue with setting an svg image inside CardMedia. Thanks @AndriusBil

My problem was that svg image was fully responsive but it wasn't working with a specified width. So I changed to maxWidth and it worked fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: card This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants