Skip to content
This repository has been archived by the owner on Apr 30, 2019. It is now read-only.

British Science Week 2017 (temporary addition) #98

Closed
wants to merge 4 commits into from

Conversation

shaunanoordin
Copy link
Member

PR Overview

  • Requested by @mrniaboc on behalf of the British Science Association.
  • For the period of... now, until 19 March 2017, the Penguin Watch website will showcase a special "challenge" banner.
  • The banner will show a specific message, in the vein of "X classifications made during British Science Week - let's aim to do 250,000 before 19 March!"
  • Which... since this is the 6th... technically makes it longer than a week. But surely there are scientific reasons for that.
  • NOTE: This is meant to be a temporary addition to Penguin Watch. Please do not merge with master; this branch will be manually deployed once Reviewers give an OK. After the challenge period, the master branch will be manually re-deployed.

screen shot 2017-03-06 at 17 44 09

Notes

  • Please note that there is very, very likely an issue with building this project. As noted in npm install fails when trying to install image-min haw#8 , there's dependency missing in the dependency chain which may make it difficult (if not impossible) to build. ( penguinwatch -> zooniverse-readymade -> haw -> image-min)
  • Currently, looks OK on localhost with OSX+FF/Chrome/Safari - fingers crossed. Several warnings while running npm install, though
  • If some time can be spared, I'd like to request @eatyourgreens's eyes on this, due to aforementioned haw issue.

@shaunanoordin
Copy link
Member Author

PR UPDATE: WELL FUUUCCC-

  • WE SHOULD HAVE HEEDED THE WARNINGS! WE WERE WRONG. WE WERE SO VERY WRONG!
  • So anyway npm start works fine if you're just looking at Penguin Watch on localhost.
  • However as soon as you try to do npm run beta or npm run build that haw issue rears its ugly head - image-min is effed up, y'all. The build dies in mid-compile, and nothing gets deployed.

Workaround?

  • I've discovered a potential (albeit messy, so very messy) workaround that will allow us to sort this out.
  • In the local /node_modules/haw/lib/default-config.coffee, there's the following...
  # Optimize files after a build.
  # Paths are rooted at the build directory.
  limit: 3

  optimize:
    '/main.js': (file, callback) ->
      UglifyJS = require 'uglify-js'
      fs = require 'fs'

      {code} = UglifyJS.minify file
      fs.writeFile file, code, callback

    '/main.css': (filename, callback) ->
      fs = require 'fs'
      CleanCSS = require 'clean-css'

      fs.readFile filename, (error, content) ->
        if error?
          callback error
        else
          min = new CleanCSS(keepBreaks: true).minify content
          fs.writeFile filename, min, callback

    '{*,**/*}.jpg': (filename, callback) ->
      Imagemin = require 'image-min'

      imagemin = new Imagemin()
        .src filename
        .dest filename
        .use Imagemin.jpegtran progressive: true
        .optimize callback

    '{*,**/*}.png': (filename, callback) ->
      Imagemin = require 'image-min'

      imagemin = new Imagemin()
        .src filename
        .dest filename
        .use Imagemin.optipng optimizationLevel: 7
        .optimize callback
  • Two options: either [A] replace image-min with an alternative library, or [B] delete the lines of code corresponding with .png and .jpg and accept potentially huge image assets on the website. (For reference, we should expect 1.4MB of media assets, in total, if completely uncompressed.)
  • For a test, I've removed the image-min dependencies on my local /node_modules/haw/ (i.e. option [B] ) and then ran npm run beta. This seems to work: https://www.penguinwatch.org/beta/

This is obviously sub-optimal, but it's a solution given the constraints that we're facing. I'm keen to hear thoughts on this.

@eatyourgreens
Copy link
Contributor

zooniverse/haw#9 might work. I'm not sure how to test it.

@eatyourgreens
Copy link
Contributor

Nice work on the banner by the way 👍

@eatyourgreens
Copy link
Contributor

70d543d should fix the haw build errors (fingers crossed.)

@shaunanoordin
Copy link
Member Author

Thanks @eatyourgreens ! I have an idea for testing zooniverse/haw#9 and solving the issue for Penguin Watch:

  1. I've git cloned haw with your latest (PR#9) updates and set it up with npm install.
  2. I copied/installed the following items into my /penguinwatch/node_modules:
  • haw (basically the whole updated repo directory)
  • imagemin
  • imagemin-jpegtran
  • imagemin-optipng
  1. I ran npm run build and the build succeeds.
  2. I ran npm run beta and the build+deploy succeeds: https://penguinwatch.org/beta

In short, the updated haw looks great!

EDIT: Just saw your use Use updated haw commit; I'll test again with a clean npm install to make sure this works properly.

@eatyourgreens
Copy link
Contributor

If that works then I'll merge zooniverse/zooniverse-readymade#40 and publish a new version.

@shaunanoordin
Copy link
Member Author

@eatyourgreens aaawww yeah, it works. 👍

To confirm:

  1. I've deleted node_modules and reinstalled via npm install (package.json is using zooniverse/zooniverse-readymade#upgrade-haw)
  2. I ran npm run build and npm run beta; both are functioning correctly.

This is looking good! I'm confident enough now to do a production deploy for Penguin Watch. I'll follow up with you shortly about what needs to be done to correctly propagate the haw/zooniverse-readymade update across other affected repos.

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Mar 7, 2017

@shaunanoordin published as zooniverse-readymade 1.5.0

@mrniaboc
Copy link
Contributor

mrniaboc commented Mar 7, 2017 via email

@eatyourgreens
Copy link
Contributor

I've just updated master to add German translations, so this will need rebasing.

@penguintom79
Copy link

penguintom79 commented Mar 7, 2017 via email

@shaunanoordin shaunanoordin force-pushed the british-science-week-2017 branch from 70d543d to 7d252ba Compare March 7, 2017 14:14
@shaunanoordin
Copy link
Member Author

GitHub Code Note: In case any fellow dev in the year 3010 is trying to follow what happened in this convo, note that this branch was rebased onto a new master that incorporates PR #99

The root of the problem was that the previous master was using "zooniverse-readymade": "~1.1.3", which had issues. The new/updated master, which this branch is rebased on, correctly uses "zooniverse-readymade": "~1.6" in package.json

Also, fellow dev of the year 3010: say hi to our robot overlords for me! Please tell them 01010100 01101000 01101001 01110011 00100000 01110011 01110100 01100001 01110100 01100101 01101101 01100101 01101110 01110100 00100000 01101001 01110011 00100000 01100110 01100001 01101100 01110011 01100101 00100001

@shaunanoordin shaunanoordin force-pushed the british-science-week-2017 branch from 7d252ba to 931baef Compare March 8, 2017 15:49
@shaunanoordin
Copy link
Member Author

This has been rebased again to accommodate some text changes in #100

@shaunanoordin
Copy link
Member Author

@mrniaboc https://www.penguinwatch.org/ has been updated with the new link! Your code update was good, by the way; it's just that instead of merging to master, we'd just npm run deploy in this case. 👍

@mrniaboc
Copy link
Contributor

mrniaboc commented Mar 10, 2017 via email

@shaunanoordin shaunanoordin force-pushed the british-science-week-2017 branch from 84af978 to be06384 Compare March 10, 2017 17:12
@shaunanoordin
Copy link
Member Author

Rebased again to reflect new information for schools, as added in #101

Wait, that PR was one hundred and one? Damn, should have named it Education 101

@eatyourgreens eatyourgreens force-pushed the british-science-week-2017 branch from be06384 to 2e75ab5 Compare March 22, 2017 12:26
@shaunanoordin
Copy link
Member Author

PR Update: Closing Time

Fiona just gave the clear to remove these temporary additions to Penguin Watch. As of right now, https://www.penguinwatch.org/ has been reverted back to its original non-BSW incarnation.

This PR will be closed, but hopefully preserved in case similar temporary modifications are required in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants