Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build improvements, asset imports, local static site serving #30

Merged
merged 13 commits into from Aug 3, 2018

Conversation

@bobheadxi
Copy link
Contributor

bobheadxi commented Aug 2, 2018

This is based on #27, that needs to merge first. The diff is really confusing cuz i messed up. Recommend reviewing this commit-by-commit starting at e443e8d

馃懛 Changes

Saw some stuff in the other PRs where images, assets were src'd via relative paths, which won't work unless we keep everything in docs/ and keep the paths set up right... blah blah it's a bit of a hassle. So we let webpack handle it:

import logo from '../../assets/logo.png';

const getLogo = () => (
  <Link to="/"><img alt="" src={logo} /></Link>
);

this is set in the webpack config:

      {
        // Package any static assets
        test: /\.png($|\?)|\.jpg($|\?)|\.ico($|\?)|\.otf($|\?)|\.woff($|\?)|\.woff2($|\?)|\.ttf($|\?)|\.eot($|\?)|\.svg($|\?)/,
        loader: 'url-loader',
      },

while I was at it, I did some other adjustments and whatnot to the makefile, and added a script to emulate serving a static web app, just in case webpack-dev-server behaves differently (ie the logo example above)

also added a thing to make sure that react-router works properly with gh-pages based on a ticket I found, but we'll have to keep an eye on if it works. More details in corresponding commit

also bumped dependencies

馃敠 Testing Instructions

make build
make serve
# go to localhost:8081
@bobheadxi bobheadxi requested review from chamkank and mingyokim Aug 2, 2018
@bobheadxi bobheadxi changed the base branch from master to cloud-functions Aug 2, 2018
@bobheadxi bobheadxi changed the base branch from cloud-functions to backend-rework Aug 2, 2018
@bobheadxi bobheadxi changed the base branch from backend-rework to master Aug 2, 2018
Makefile Outdated
.PHONY: serve
serve:
@echo Go to http://localhost:8081 to test the built web app
@(cd ./docs ; python ../serve.py)

This comment has been minimized.

Copy link
@chamkank

chamkank Aug 2, 2018

Member

Can we use http-server instead of the Python script? I know Python 2 & 3 come installed by default on Macs but that's not the case for Windows. After running yarn add http-server -D in web, we can replace line 60 in the Makefile with @(./web/node_modules/.bin/http-server ./docs -a localhost -p 8081 -c-1).

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 2, 2018

Author Contributor

Ah, I was hoping to avoid installing more npm dependencies, and since we might be using python for mock data filling, I was hoping we could just get the ball rolling here.

If it is a significant inconvenience, I could make the switch 馃憤

This comment has been minimized.

Copy link
@chamkank

chamkank Aug 2, 2018

Member

That would be nice because requiring the user to have Python 2 installed is more of an inconvenience than installing an additional npm package in my mind. Also avoids having to create a new file just for serving the app. Is there a reason why we wouldn't do mock data filling in JavaScript? Imo it would be really nice to stick to one language unless we have a compelling reason not to.

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 2, 2018

Author Contributor

No reason, one language is a bit boring and I just find python more convenient, and python2 is here because as you mentioned its installed by default on osx. But javascript is fine too.

Fair point on the extra file (though extra npm dependency 馃槩 ) - i'll make the changes tonight to use the npm package instead

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 3, 2018

Author Contributor
@@ -1,43 +1,84 @@
all: deps

# List all commands
.PHONY: ls

This comment has been minimized.

Copy link
@chamkank

chamkank Aug 2, 2018

Member

Woohoo this will be useful!

@bobheadxi bobheadxi requested a review from chamkank Aug 3, 2018
@bobheadxi bobheadxi changed the title Web/build Build improvements, asset imports, local static site serving Aug 3, 2018
Copy link
Member

chamkank left a comment

馃憣

@bobheadxi bobheadxi merged commit 40bbbbf into master Aug 3, 2018
4 checks passed
4 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@bobheadxi bobheadxi deleted the web/build branch Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.