newhavenio / newhavenio.github.io

active version of the website for newhaven.io built on the Jekyll framework
http://newhavenio.github.io/
MIT License
13 stars 12 forks source link

Feature/bundle js with webpack #50

Closed bsutt123 closed 6 years ago

bsutt123 commented 6 years ago

Included a basic webpack and bundle.js configuration to get started. Other small points

1) added node_modules to .gitignore and excluded from jekyll again. 2) bundle.js is not ignored because we will have to include that to get the website working on deploy. In an ideal world, GH pages runs npm run build before building the jekyll page but I don't know if we can make it that smart.

thatnerdjosh commented 6 years ago

@bsutt123, either myself or @treznick are available to pair on this to get it ready to merge, all good changes so I personally would like to see this in the repo after some cleanup

bsutt123 commented 6 years ago

I've been a bit busy with outside projects this week. So it looks like most of the requested changes are whitespace and small typing errors. Is it better to go back and revert to commits that only include the right whitespace, or make a new commit on top of what I have that fixes the whitespace but doesn't remove the original commits from the history?

Just curious which way is best.

thatnerdjosh commented 6 years ago

@bsutt123 it is commit splitting, commit message changes, rebasing and whitespace yeah, everything else basically seems good afaict. Also since this is on a separate branch, rewrite history as much as you need :) it is only once merged we shouldn't rewrite history.

bsutt123 commented 6 years ago

honestly, probably best to just figure out a time to pair on it, I haven't done that much before and I would probably just insert more problems.

thatnerdjosh commented 6 years ago

Message me on slack username is Josh Santos

bsutt123 commented 6 years ago

Anytime after 8 works for me. Or earlier on some days if you don't stay up till 8.

thatnerdjosh commented 6 years ago

@treznick This looks much better now I worked with @bsutt123 to clean stuff up, if you could review that'd be perfect, we split up the commits logically

bsutt123 commented 6 years ago

Build now includes @NerdsvilleCEO work to use Travis CI to build. Merged into my branch on my branch

bsutt123 commented 6 years ago

So whats going on with this? Is there something we need to add or change?

jnimety commented 6 years ago

Has the Readme been updated with new pre deploy instructions?

bsutt123 commented 6 years ago

So @NerdsvilleCEO added in TravisCI so that there isn't any pre-deploy instructions, the github pages deploy should take care of it. Josh added that as the later part of the commit because people were worried about the bundle.js needing to get built before deploying.

I added in a part to the readme that reminds you to do a build step locally if you want to make changes to index.js which is the entry point for the webpack build.

jnimety commented 6 years ago

oh, right! This might be ready then, would you mind looking it over to double check things?

bsutt123 commented 6 years ago

I'll pull it down and run through it one more time just to be sure that everything is tip top shape.

bsutt123 commented 6 years ago

switched to feature/bundle-js-with-webpack on my local computer and ran the following commands...

bundle install
yarn 
npm run build
jekyll serve

No errors, and the site was fully funcitonal. I'm not sure how to tell it to follow the TravisCI protocol directly, or I would have done that.

jnimety commented 6 years ago

@tom @NerdsvilleCEO do we have to do anything to enable the travic-ci deploy once we merge?

treznick commented 6 years ago

@jnimety I think it's probably good to go. I say we try it and if it blows up we can revert the build.

bsutt123 commented 6 years ago

Is everyone ready to pull the trigger?

Also side note, the prodcution site can't currently find slack-button.html so that bug is coming from master. I'll see If i can track it down.

UPDATE: slack-button.html is referred to by masthead.html but not included in any component directory. I have a feeling that its an old bit of code that didn't get removed when we remade the slack button a few commits back. I can remove it with this PR but because its already approved I would rather do it with the next one.

ZachBeta commented 6 years ago

@bsutt123 please smoke test and confirm

ZachBeta commented 6 years ago

the event list at the bottom seems to be failing, but idk if that is on my end, or was from this PR, or is intermittent, or what

hard refresh: working now