astro / prittorrent

BitTorrent Content Distribution for Podcasts
http://bitlove.org/
93 stars 19 forks source link

Made flattr buttons only appear on hover, fixed indentation in README #14

Closed willi-mueller closed 12 years ago

willi-mueller commented 12 years ago

I tested it only on comparable html/css files because I could not manage to setup postgres to run bitlove. Please test it on your setup to be sure.

Apart from that I made the functions in flattrdropdown.js not polluting global namespace.

In the README file the feature request I implemented is striked through now. Is this a reasonable approach to keep track of things in the bitove project?

willi-mueller commented 12 years ago

If you don't like that merging mess I could pull freshly from your master and squash my stuff into one commit ;-)

astro commented 12 years ago

Merging is fine.

I'm still not sure on 9b27e84. It reduces readability without a benefit. On the contrary, I would not be able to call loadFlattr() from other scripts. This is not a library for reuse but a web service.

Perhaps I'm going to cherry-pick just 54c8e7b...

willi-mueller commented 12 years ago

In 3b86f380 I also removed the now useless .shown class. Would you appreciate a refactoring to the already included jQuery instead of native DOM manipulation. It feels a little old school for my youngish eyes ;-)

astro commented 12 years ago

Only if the DOM code is lacking portability in some way. jQuery clocks in at 32 KB and I am not going to use a 3rd party CDN.

willi-mueller commented 12 years ago

Ok, I just wondered because jQuery is already in the repo. If bandwidth is a problem I suggest scaling down the podcast logos (Fanbóys = 240kB, CRE = 220kB, AppSacker = 200kB) and cache them. But it may be premature.

astro commented 12 years ago

Yes, I plan to cache & resize them eventually. Go for it if you want to learn Erlang. For now I'm concetrating on what I deem the most important features.

Don't be discouraged. There are plenty of TODO items in the README.md file. It's just that there is no real-world justification in merging the commits in this pull request. Perhaps I am going to stop polluting the global namespace once I know I don't need the functions in other scripts. I also have started using jQuery for Podcaster Login in the dev branch. That fraction of the user base can take a few additional milliseconds.

willi-mueller commented 12 years ago

I agree with you that this caching thingy isn't the most valuable thing for users right now. But can you prioritize the TODOs for people who want to dive in?

I'm not discouraged ;-) and I already have some Erlang experience with Webmachine and I really like to work on the Login/Session stuff and I think I can help there at the frontend as well.

Why don't you want to merge the flattr on hover CSS + the one removed line of JS which adds the "shown" class? Isn't it what you expected? How can it be improved?

astro commented 12 years ago

Flattr drop down has been replaced by the original buttons again.

willi-mueller commented 12 years ago

Looks much better than before. Cool