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

[READY] README demo for defining availableWidths as a function does not work #66

Merged
merged 3 commits into from
Feb 3, 2014

Conversation

thom4parisot
Copy link
Contributor

The readme suggests that I can pass a function to availableWidths, however the example provided does not appear to work as expected.

Codepen Demo

As an aside:

What is the use-case for defining availableWidths as a function? I know it allows people to customize the function that runs when replaceImagesBasedOnScreenDimensions is called, but surely that's core functionality to Imager? Anybody that wants to overwrite it would also have to reimplement determineAppropriateResolution themselves otherwise they would end up hammering their image service on every resize. To me this feels like we're giving people the option to reimplement core internals at a too deep level without an obvious reason why they would need to.

@BPScott BPScott mentioned this pull request Jan 6, 2014
@thom4parisot
Copy link
Contributor

This started here:

I think it might actually be worth making a new image every 8 pixels, from what I can tell by the RespImages community chatter JPEG has some magic formula where images that are factors of 8x8 compress much better than other dimensions.

So instead of having a long array, the idea was to calculate dynamically the width of an image.
That's basically what is tested here:

https://github.com/BBC-News/Imager.js/blob/935089d0a829a425cef06b34ef332d0933744b57/test/unit/core.js#L126-133

The idea is that Imager still works the same way, but the width picker logic can be overridden for special needs.
It has to just return a value. This is the only responsability of that function, simple.

Thomas Parisot and others added 3 commits January 23, 2014 10:48
Running SauceLabs on branch, locally otherwise
- removed `isArray` function
- bootstrapping instance variables
- array/object paths
@Integralist
Copy link
Contributor

@oncletom heya, just checking if this ready for me to review (looks like it)?

@thom4parisot
Copy link
Contributor

@Integralist yep, ready :-) it was an "almost" working feature, previously badly implemented.

Integralist added a commit that referenced this pull request Feb 3, 2014
[READY] README demo for defining availableWidths as a function does not work
@Integralist Integralist merged commit 056c238 into bbc:browsers-compatibility Feb 3, 2014
@thom4parisot thom4parisot deleted the fix-66 branch February 3, 2014 10:40
@BPScott
Copy link
Contributor Author

BPScott commented Feb 3, 2014

It looks like this got merged into the browsers-compatibility branch, not master. Should this have gone to master as browser-compatability was already merged?

@thom4parisot
Copy link
Contributor

@BPScott I'll rebase as soon as I end up the current meeting :-)

@thom4parisot
Copy link
Contributor

@BPScott okay merged in master :-)

@thom4parisot thom4parisot restored the fix-66 branch February 5, 2014 11:48
@thom4parisot thom4parisot deleted the fix-66 branch February 5, 2014 11:49
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.

None yet

3 participants