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

[Opinionated] Add alt attribute & remove container #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kurtextrem
Copy link
Contributor

I've added an alt attribute, because "Placeholder" and "Worker" as alt is not really nice.
Second thing I've done is remove the container, because if you're using <ImageWorker> as replacement for <img> it should not become a <div>.

);
const { style, imageClass, alt } = this.props;
const { isLoading, imgSrc } = this.state;
return state.isLoading ? this.renderPlaceholder() :
Copy link
Owner

Choose a reason for hiding this comment

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

isLoading will be an unused variable.. you can change it to the following

return .isLoading ? this.renderPlaceholder() :
  <img src={imgSrc}
      style={{ ...style }} className={imageClass} alt={alt} />

@nitish24p
Copy link
Owner

nitish24p commented Feb 10, 2018

So i converted it to a div because if someone supplies a placeholder as a div and then the image loads, so we will toggle from a block element to an inline element, and that may break UI. Hence i thought of wrapping it in a block element div regardless. If one wants to make the parent an inline then you can add a containerClass prop.@manjula91

display: inline-block

@manjula-dube
Copy link
Collaborator

Looks Good

@nitish24p I think we can merge it :)

@nitish24p
Copy link
Owner

One comment, and eslint..

@nitish24p
Copy link
Owner

can you run npm run eslint to see the lint errors and fix those

@kurtextrem
Copy link
Contributor Author

Fixed!

But I think it should be added to the readme, because as I said I'd expect an <img> replacement to be inline too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants