search

bbc / Imager.js

Responsive images while we wait for srcset to finish cooking
Apache License 2.0
3.84k stars 224 forks source link

Strategies, URL variables and Unit Testing #15

Closed thom4parisot closed 11 years ago

thom4parisot commented 11 years ago

tl;dr

Heavy documentation and logic splitting. We can plug several way to replace the <div> container.

New Stuff

Basically, they are all about opinionated software design:

^ responsive images huh?

thom4parisot commented 11 years ago

Open for reviews @maslen @Integralist @addyosmani :-)

thom4parisot commented 11 years ago

@Integralist I've rebased on the latest master; and even adjusted the new demo page :-) working like a charm!

addyosmani commented 11 years ago

Whoa. Massive work! Will review the commits today and tomorrow. Thanks for your time on this @oncletom!

addyosmani commented 11 years ago

adding @sindresorhus for any comments on the review too. Still looking through.

sindresorhus commented 11 years ago

I like this change, especially the configurable URL patterns and pluggable event system :)

configurable delay to postpone width URL calculation

Why is this needed?

Atomic commits are good, but this many commits just counteracts the intention and just makes me look at the complete diff. Interactive rebase is your friend ;)

thom4parisot commented 11 years ago

@sindresorhus well, I kept the delay thing after seeing in the ImageEnhancer code, and talking with @kaelig; where the most pragmatic thing is to favour loading the other components of the page then loading the additional content. Especially if you are changing the device orientation, it's better to make sure the phone had time to resize all the contents prior to eventually loading the new responsive URLs.

In this case, regarding the atomic commits, I did not know if I was going to be able to create multiple PR, for every specific feature… I progressed in a very transversal way, making things complicated to decouple within the Git history. Sorry about that, I wish I could have done that a cleaner way.

thom4parisot commented 11 years ago

@sindresorhus @addyosmani @Integralist hi guys, I'm back! Adjusted things following your advices (codestyle consistency, doc, "BBC" strategy name).

Do you have other blockers, improvements or suggestions?

sindresorhus commented 11 years ago

lgtm

Integralist commented 11 years ago

@oncletom heya, thanks for taking the time to work on this. I'm going to review it this weekend (apologies for taking so long to get around to it).

One thing I would like to see is some form of documentation/written example.

Also I'm wondering whether the current implementation should be kept in a sub folder as a simplified version for users? Admittedly I've not been through your code yet but the impression I'm getting is that the current implementation is smaller and offers users an easy route to get started and would be good to keep as an alternative if they didn't want the "all singing, all dancing" version you've worked on in this PR.

@addyosmani your thoughts on my above comments would be appreciated.

thom4parisot commented 11 years ago

@integralist well, the Readme doc is not complete enough? I documented easy and custom routes to use it.

On an end user point of view, the only difference is to do yourself the dom selection, and to hook yourself on events. Which is documented and as complicated as listening to a click event.

If there is room to simplify things I'll do it.

Integralist commented 11 years ago

Sorry @oncletom didn't notice the README had been updated. Was going to review properly on Sunday.

thom4parisot commented 11 years ago

@Integralist cool, let me know in any case :-)

thom4parisot commented 11 years ago

Updated a couple of things to generate min output and to tweak some bytes.

screen shot 2013-09-07 at 21 58 48

Integralist commented 11 years ago

@oncletom @addyosmani I've had some time to go through this PR and to consider whether it's appropriate to the focus of the project, and my feeling is that (although this adds a couple of potentially useful additions) the new features introduce a complexity which could be accomplished in a simpler fashion (the sheer size of the PR and the number of changes are indicative of this).

To make sure it wasn't just myself seeing it this way I consulted with a couple of the team members here at BBC and also with the W3C Responsive Images Community Group to gauge feedback of this PR and it seems the added complexity was an issue but the ultimate opinion shared by all (both BBC team and community) was that this should stay as a forked version of the current Imager.js code.

What I would then suggest is that I update our README to point to Thomas' forked repo's (similar to how Underscore and Lodash co-exist) so developers can make the decision on which version they prefer to use.

In the background I can begin work on implementing some of the new additions (re: pixel density specification and handling dynamically added placeholders) in a way I feel is more appropriate for this project.

I'm going to close this PR but feel free to continue on the discussion below. But like I say I've gone with the majority decision (especially considering the W3C RICG are the exact audience this repo is targeting).

Obviously this goes without saying, but thank you for the amazing work you've done on this PR and hope you're OK with my suggestion to point to your repo's as an alternative option for users.

thom4parisot commented 11 years ago

Too bad you made this decision without talking with me.

Waste of time.

kaelig commented 11 years ago

Well at least I learned the basics of Angular while building this configuration tool http://kaelig.github.io/Imager.js-customizer/ :)

Integralist commented 11 years ago

@oncletom If you you can simplify the work you've done then obviously I'll be happy to re-review.

I consulted with the community who are the target user audience for this library, they and other members of the team felt a fork was more appropriate (which matched my own opinion).

Again, I very much appreciate the time you've spent, but as I'm sure you're aware: doing a PR on an open-source project doesn't automatically guarantee it'll be excepted.

This shouldn't be considered a waste of time as I'll be making mention of your work in the readme file so developers know they have an option they can choose from.

@kaelig always a silver lining ;-)

DavidBruant commented 11 years ago

I consulted with the community who are the target user audience for this library

It might be appropriate to document the project governance in the Readme. For instance "major changes have to be discussed in <link to mailing-list> first". You also don't provide a link to this consultation. I can't find it on the public mailing-list, so I imagine it happened behind closed doors. I have no problem with this form of governance but it'd be nice if that was told upfront so people know about it.

I can understand @oncletom's frustration when after a 14 days PR with 55 commits, the PR is closed without a single warning, without discussion.

After someone does 55 commits within 2 weeks of work, following the contribution guidelines, asking for feedback, iterating several times, it might have not been entirely crazy to keep him in the loop when making the decision of what to do with his work. WE ARE FUCKING HUMAN BEINGS FOR FUCK SAKE!

What I would then suggest is that I update our README to point to Thomas' forked repo's (similar to how Underscore and Lodash co-exist) so developers can make the decision on which version they prefer to use.

The comparison with Lodash is inaccurate since @oncletom hasn't expressed an intent to maintain a fork of Imager.js.

This shouldn't be considered a waste of time as I'll be making mention of your work in the readme file so developers know they have an option they can choose from.

Since the fork won't be maintained (I haven't talked with Thomas about it yet, but I imagine he doesn't plan to maintain it), evolution of the BBC-News versions won't be ported and Thomas' fork will very soon fall behind and won't be a viable choice for developers. So what you're saying is true, but only for every short period of time. Probably not the sort of impact Thomas was looking for on this project.

Despite what your reassuring words, his time has been effectively wasted; it's hard to find more accurate words. Reviewers time too for that matter!

the sheer size of the PR and the number of changes are indicative of this

I'm not familiar enough with the project, but I can agree with this. Still, it might be worthwhile for you as project maintainer to discuss change by change with Thomas and ask to re-submit them in several independent smaller PRs (did I write something in full-caps about "fucking human beings" already?). It is absurd to reject 100% of a big PR like that. I can't believe none of the 55 well-cut commits, all are to be thrown away or belong to a different fork. To quote the contributing guidelines:

Make sure commits are as 'atomic' as possible (this way specific changes can be removed or cherry-picked more easily)

This seems like a way for your project to benefit from Thomas' work. Not sure what else to say. It's all in your guidelines...

doing a PR on an open-source project doesn't automatically guarantee it'll be excepted.

Glamorous typo.

thom4parisot commented 11 years ago

Got angry. Had a breath. Had some cheese. Got better

brunobord commented 11 years ago

/me is handing a http://beeroverip.org/random to @oncletom

thom4parisot commented 11 years ago

So final words. Emotional reaction and frustration as a drawback of a passionate work ;-)

We will talk about this PR to design features independently from each other.

Thanks @brunobord for the beer. I had wine instead but it worked too ;-)