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

Preload placeholder image not being properly displayed - Fix issue #23 #25

Closed
wants to merge 1 commit into from

Conversation

davidrissato
Copy link

Fix issue #23

@davidrissato davidrissato changed the title Fix issue #23 Preload placeholder image not being properly displayed - Fix issue #23 Nov 6, 2015
@bicknellr
Copy link
Contributor

Could you add a test or two for this?

@davidrissato
Copy link
Author

Ok, I'll add them in a couple days.

@davidrissato
Copy link
Author

@bicknellr I'm trying to implement some tests to this problem, but I believe I need to go deep in the shadowed elements to expose the problem. I'm not an expert in HTML tests. Can I do that? I think if I do this way my tests will fail in case of internal refactoring.

@bicknellr
Copy link
Contributor

I've been trying to group together a handful of fixes in #44 and I've added a test there for the issue this PR is addressing. I think you're right, there doesn't appear to be a good way to test this without reaching into the internals. :/

@davidrissato
Copy link
Author

@bicknellr Yes. So I'm a little bit confused. Will you use this pull or have you already added these changes in #44? Do you still need that I add tests here?

@bicknellr
Copy link
Contributor

@davidrissato Sorry about stepping on your toes here! It seemed like a few different issues could be resolved by refactoring iron-image to use a single img element internally - #23 being one of them - so don't worry about pushing tests for this PR. However, it would be great to get your confirmation that #44 correctly tests for / fixes #23.

@davidrissato
Copy link
Author

Ok, no problem. This solution was based on a single img element too, so I believe you are addressing it as well. I'll check #44 and verify if it really fixes all #23 reported problems.
By the way, should we close this pull request?

@bicknellr
Copy link
Contributor

Sounds good, thanks!

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

Successfully merging this pull request may close these issues.

3 participants