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

Add the possibility to configure the placeholder #143

Closed
wants to merge 1 commit into from
Closed

Add the possibility to configure the placeholder #143

wants to merge 1 commit into from

Conversation

ioanlucut
Copy link

ref #142: Add the possibility to configure the placeholder only during loading of the image (when the lqip is shown)

Changes related to #142

renderNoscript(props) {
return props.ssr ? (
renderNoscript() {
return this.props.ssr ? (
Copy link
Owner

Choose a reason for hiding this comment

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

this change is unrelated to the feature. Idea is to move to hooks from classes, so this code will be changed back eventually.

Copy link
Author

Choose a reason for hiding this comment

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

The only reason was to use the isLoaded function/getter properly in all other cases, to be consistent.

@@ -148,17 +153,18 @@ export default class Media extends PureComponent {
<div
{...compose(
theme.placeholder,
!this.isLoaded && theme['placeholder--loading'],
Copy link
Owner

Choose a reason for hiding this comment

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

there are other icons, which will get blurred: Loading | Network error | 404 error | Offline

https://github.com/stereobooster/react-ideal-image/blob/master/introduction.md

Copy link
Author

Choose a reason for hiding this comment

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

True, thanks for pointing it out. Need to think a bit about it.

…g loading of the image (when the lqip is shown)

Also, remove the jest threshold as it is not met and it breaks the build
@ioanlucut
Copy link
Author

Any idea why Travis is keep failing? Running the tests locally looks green.

@stereobooster
Copy link
Owner

Any idea why Travis is keep failing? Running the tests locally looks green.

bug in jest. master branch also fails

@ioanlucut
Copy link
Author

bug in jest. master branch also fails

Why does pass locally, then?

@ioanlucut ioanlucut changed the title ref #142: Add the possibility to configure the placeholder Add the possibility to configure the placeholder Mar 24, 2019
@stereobooster
Copy link
Owner

jestjs/jest#5270

@ioanlucut ioanlucut closed this Sep 5, 2019
@ioanlucut ioanlucut deleted the placeholder-active branch September 5, 2019 05:56
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.

2 participants