Skip to content

[Opinionated] Add alt attribute & remove container#13

Open
kurtextrem wants to merge 3 commits intonitish24p:masterfrom
kurtextrem:feat-3
Open

[Opinionated] Add alt attribute & remove container#13
kurtextrem wants to merge 3 commits intonitish24p:masterfrom
kurtextrem:feat-3

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