TechAtNYU / tech-nyu-site

https://techatnyu.org
8 stars 5 forks source link

Ls to js, no pagination mode, performance refactors #70

Closed ethanresnick closed 9 years ago

ethanresnick commented 9 years ago

As the title implies, this PR removes all the Livescript from the site's codebase; takes out the desktop design's "pagination mode", which wasn't worth the performance hit (and would've been increasingly constraining as we made more of the sections taller); and includes some other performance-related enhancements.

THIS IS NOT QUITE READY TO MERGE. STILL SOME RANDOM NEW BUGS. But you can test it out if you're curious, and/or familiarize yourself with the site's code now that it's finally in something standard/readable.

cc @AbhiAgarwal @grungerabbit @danagilliann @oa495 @terriburns @maxdumas

terriburns commented 9 years ago

omg

ethanresnick commented 9 years ago

@terriburns yup. it looks like more than it is, since most of these changes are from just compiling the livescript and cleaning up the result (and the whole codebase was livescript). but still, i'm excited to see this land once the bugs are worked out (esp the pagination stuff)—should make the code way easier to understand/debug/modify.

terriburns commented 9 years ago

@ethanresnick do we still run it using gulp --require gulp-livescript ?

ethanresnick commented 9 years ago

@terriburns no, that's not necessary any more. Good catch! I'll update the readme when I'm next at a computer.

I'm also hoping I can remove our dependency on sass 3.3; we're currently locked into that version because we're doing some non-standard (read: bad, hacky) stuff that sass dropped support for. Removing this hack will let us update sass and, more importantly, offer a further perf speedup

revivek commented 9 years ago

@ethanresnick small other point regarding performance: not sure if the stylesheets are complex enough to result in slow compilation, but if possible I would suggest switching from gulp-ruby-sass (requires Ruby, uses Ruby to compile) to gulp-sass (uses node, uses C to compile) for a order-of-magnitude compilation speed increase.

That several-second increase matters for us (Oyster). Not sure if it'd be noticeable for you all, but wanted to throw this out there.

ethanresnick commented 9 years ago

@revivek oh how nice it would be if the performance concern were just sass compilation time :) There's actually something way more sinister going on here, which is that we're synchronously fetching a pseudo-stylesheet on the client, parsing it with js, and using that to add animation instructions to the dom. That pseudo-stylesheet, which is currently generated by sass and has some invalid CSS formatting (the hackiness alluded to above) is what I'm trying to remove. The idea is just to deliver those animation instructions in js (why they need to be js and not normal CSS is a different story) with the main app.js download, to save the parsing step and the extra request.

After that, switching to the c sass compiler still may be a good idea, though if I rememver correctly it didn't have 100% feature parity with the ruby compiler. Is that still true? May not be an issue, though we are using some pretty fancy sass features.

revivek commented 9 years ago

@ethanresnick I see... yes, that makes sense.

There wasn't feature parity about a year ago, although I think they're pretty close now. C compiler supports at least up to 3.2 last time I checked.

ethanresnick commented 9 years ago

Ok, I'm going to give this a merge, since I don't wan the perfect to be the enemy of the good here. It still has some bugs, but I think only the same ones that are already on the current site.

AbhiAgarwal commented 9 years ago

:100:

ethanresnick commented 9 years ago

Thanks @AbhiAgarwal! Unfortunately, it looks like this merge broke the build step, since now gulp is run without livescript and through bundler). Can you investigate that?

AbhiAgarwal commented 9 years ago

@ethanresnick yup! I think I'll just have to update the circle.yml file to change the require gulp-livescript to bundle exec gulp

ethanresnick commented 9 years ago

Perfect; that should work!

AbhiAgarwal commented 9 years ago

@ethanresnick Fixed! :100:

ethanresnick commented 9 years ago

Thanks Abhi!