Strategies, URL variables and Unit Testing #15
Conversation
|
Open for reviews @maslen @Integralist @addyosmani :-) |
…ontainer tag semantic
…ten loops. Adding partial nextTick support. Optimizing loops.
… create an Imager per type of replacement. That's the job of the `querySelectorAll` code. Replacer -> Strategy to be less missleading.
Adjusting the tests which were breaking because of that.
|
@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? |
|
lgtm |
|
@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. |
|
@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. |
|
Sorry @oncletom didn't notice the README had been updated. Was going to review properly on Sunday. |
|
@Integralist cool, let me know in any case :-) |
And a couple of few tweaks and codestyle consistency.
…thinking about making this value mandatory w/o defaultsas it's business specific)
|
Updated a couple of things to generate min output and to tweak some bytes. |
|
@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. |
|
Too bad you made this decision without talking with me. Waste of time. |
|
Well at least I learned the basics of Angular while building this configuration tool http://kaelig.github.io/Imager.js-customizer/ :) |
|
@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 ;-) |
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". 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.
The comparison with Lodash is inaccurate since @oncletom hasn't expressed an intent to maintain a fork of Imager.js.
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. 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!
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:
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...
Glamorous typo. |
|
Got angry. Had a breath. Had some cheese. Got better |
|
/me is handing a http://beeroverip.org/random to @oncletom |
|
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 ;-) |
tl;dr
Heavy documentation and logic splitting. We can plug several way to replace the
<div>container.New Stuff
http://myserver.co.uk/images/{width}-hidpi/picture.jpg)Removed Stuff
Basically, they are all about opinionated software design:
$shim (we just iterate on array-like of elements)^ responsive images huh?