Skip to content
This repository was archived by the owner on Jun 24, 2021. It is now read-only.

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

Merged
bobheadxi merged 13 commits into
masterfrom
web/build
Aug 3, 2018
Merged

Build improvements, asset imports, local static site serving#30
bobheadxi merged 13 commits into
masterfrom
web/build

Conversation

@bobheadxi

@bobheadxi bobheadxi commented Aug 2, 2018

Copy link
Copy Markdown
Contributor

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 August 2, 2018 06:42
@bobheadxi bobheadxi added the status: DO NOT MERGE!!1! just for @mingyokim ❤️ label Aug 2, 2018
@bobheadxi bobheadxi changed the base branch from master to cloud-functions August 2, 2018 06:46
@bobheadxi bobheadxi changed the base branch from cloud-functions to backend-rework August 2, 2018 06:47
@bobheadxi bobheadxi changed the base branch from backend-rework to master August 2, 2018 06:48
Comment thread Makefile Outdated
.PHONY: serve
serve:
@echo Go to http://localhost:8081 to test the built web app
@(cd ./docs ; python ../serve.py)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@bobheadxi bobheadxi Aug 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread Makefile
all: deps

# List all commands
.PHONY: ls

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohoo this will be useful!

@bobheadxi bobheadxi requested a review from chamkank August 3, 2018 00:31
@bobheadxi bobheadxi changed the title Web/build Build improvements, asset imports, local static site serving Aug 3, 2018

@chamkank chamkank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@bobheadxi bobheadxi removed the status: DO NOT MERGE!!1! just for @mingyokim ❤️ label Aug 3, 2018
@bobheadxi bobheadxi merged commit 40bbbbf into master Aug 3, 2018
@bobheadxi bobheadxi deleted the web/build branch August 3, 2018 01:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants