Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

Option to stop redundant download of small images. - #77

Closed
nkp wants to merge 4 commits into
bbc:masterfrom
nkp:RegressionOption
Closed

Option to stop redundant download of small images.#77
nkp wants to merge 4 commits into
bbc:masterfrom
nkp:RegressionOption

Conversation

@nkp

@nkp nkp commented Jan 31, 2014

Copy link
Copy Markdown
Contributor

This option stops Imager.js downloading a new image when a higher
resolution copy has already been downloaded. This can occur when a
mobile device is rotated from landscape to portrait.

Upscaling to larger images is unchanged.

This option stops Imager.js downloading a new image when a higher
resolution copy has already been downloaded. This can occur when a
mobile device is rotated from landscape to portrait.

Upscaling to larger images is unchanged.
@thom4parisot

Copy link
Copy Markdown
Contributor

Thanks for the contribution :-)

Have you seen #48? I feel it has the same purpose.

@nkp

nkp commented Feb 2, 2014

Copy link
Copy Markdown
Contributor Author

I hadn't seen that issue. I agree, this commit serves exactly that purpose.

Comment thread Imager.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why doing that for IE8? A DOM element does not support JS attributes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's key to check the naturalWidth of the image as opposed to the clientWidth as it represents the image's true size, unscaled by DOM or CSS. IE doesn't support the naturalWidth/naturalHeight properties until IE9.

@thom4parisot

Copy link
Copy Markdown
Contributor

Cool so @nkp if you can remove the bits related to the "option" thing, I'll write the tests before merging that into master :-)

@thom4parisot thom4parisot added this to the 0.1.2 milestone Feb 3, 2014
Comment thread Imager.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@nkp any chance you explain this special case for IE8? (I mean why not using the DOM Element own JS property?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's key to check the naturalWidth of the image as opposed to the clientWidth or width as it represents the image's true size, unscaled by DOM or CSS. IE doesn't support the naturalWidth/naturalHeight properties until IE9.

The computed width for download is based on the scaled clientWidth of the image and as such will always be greater-than or equal to the clientWidth.

I decided that supporting IE8 was worth the trivial work around.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh ok it's a real attribute computed by the browser! I did not know it :-D

A very quick cross-browser shortcut would be to store only the biggest availableWidths value used and update the src url only if the new value is bigger.

Or not to care about IE8 for this feature? @Integralist, is the IE8 support going to last long at BBC News?

@Integralist

Copy link
Copy Markdown
Contributor

@oncletom sadly... Yes :-( it is a trivial additional piece of code but will make a big difference overall I think considering there are still lots of organisations who still need IE8 support.

@thom4parisot

Copy link
Copy Markdown
Contributor

Okay that makes sense :-) I'll merge that before the next release, adding some relevant tests at the same time (no need to rebase @nkp unless this is some delight which makes your day :-p)

@thom4parisot thom4parisot modified the milestones: 0.3.0 – Convenience features, 0.3.0 - Lazyloading Stability Mar 17, 2014
@thom4parisot

Copy link
Copy Markdown
Contributor

@nkp it is a tricky one. Hopefully writting tests for this issue raised the #83 and #86.

I expect to merge this one this afternoon, after successful covering your proposal :-)

@thom4parisot

Copy link
Copy Markdown
Contributor

@nkp I'll merge this one once the two previous quoted issues gets some feedbacks.

thom4parisot pushed a commit that referenced this pull request Mar 20, 2014
And fixes 'strict' warning for getNaturalWidth definition.
thom4parisot pushed a commit that referenced this pull request Mar 20, 2014
@thom4parisot

Copy link
Copy Markdown
Contributor

This has been merged in master.

Slight change of behaviour: the image width attribute has to bet set to make sure the natural width of the image does not prevent it to be resized.

It might be a breaking change so for now it's in master and behind the 0.3.0-beta.1 tag.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants