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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial App Import #2

Merged
merged 3 commits into from
Oct 6, 2016
Merged

Initial App Import #2

merged 3 commits into from
Oct 6, 2016

Conversation

rajadain
Copy link
Member

@rajadain rajadain commented Oct 5, 2016

Overview

Imports micro site from Model My Watershed. Sets up basic bundling into /dist.

Notes

In an effort to minimize the difference during import, I've tried to keep the scripts consistent with what they were in Model My Watershed. However, they are now inconsistent with all our other projects which run scripts from within the VM, rather than from outside. I'm making cards to address this in the future.

We probably don't need the --vendor flag in the bundling script anymore. That is also a holdover from Model My Watershed and should be removed.

Apparently we minify only JavaScript, not any of the CSS. We should look into minifying CSS. Also, I just copied over the entire SASS directory from Model My Watershed. We can probably lose a lot of that.

Maybe we shouldn't be exporting to the /dist folder and serving from there for development? This was the easiest thing to do right now, but I feel /dist should only be used for production builds.

Even though this is going to be on a CDN, there's still a lot of files being requested. We should look into using CSS rather than Retina.js for serving higher resolution images to reduce the number of requests, and maybe also look into CSS sprites if that is not enough.

Testing Instructions

Check out this branch. Reload Vagrant so it picks up on new server port mapping. Then run the bundle script:

$ ./scripts/bundle.sh --vendor

Open localhost:8000 in a browser and test that the app works identically to the one on staging, and that there are no errors in the console.

Connects WikiWatershed/model-my-watershed#1483

@jtarricone
Copy link

The app behavior looked fine, but it might be notable that npm install choked when invoked from setup.sh. I have the npm log if it would be of interest.


ARGS=$*

vagrant ssh -c "cd /vagrant/src/app/ && npm $ARGS"
Copy link

Choose a reason for hiding this comment

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

Is this script necessary any longer, considering that we now have update and server?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed npm.sh

@@ -19,6 +19,6 @@ then
then
usage
else
npm run start
vagrant ssh -c "cd /vagrant/dist/ && python -m SimpleHTTPServer 8000"
Copy link

Choose a reason for hiding this comment

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

Is there a reason we need to execute this outside of the VM? The convention we have been following for these scripts (update, server, etc.) has been to execute them within the context of the VM, so we shouldn't need to wrap these commands with vagrant ssh ..., right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed vagrant ssh wrapper, this is now expected to be run from within.

"type": "git",
"url": "git+https://github.com/WikiWatershed/mmw-micro.git"
},
"author": "Azavea Inc.",
Copy link

Choose a reason for hiding this comment

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

Consider changing author to just "Azavea".

@@ -21,6 +21,6 @@ then
else
./scripts/update.sh
./scripts/test.sh
npm run bundle
./src/app/bundle.sh --vendor --minify
Copy link

Choose a reason for hiding this comment

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

I think the bundle script should be called from update. Maybe we can use a "production" environment variable to indicate that minification needs to happen.

@kdeloach
Copy link

kdeloach commented Oct 5, 2016

Can we change this so that running ./scripts/update.sh; ./scripts/server.sh works as expected, instead of having to run ./scripts/bundle.sh --vendor?

@rajadain rajadain force-pushed the feature/tt/initial-app-import branch from d997b87 to d07934a Compare October 5, 2016 21:24
@rajadain
Copy link
Member Author

rajadain commented Oct 5, 2016

Updated according to all feedback.

Starting with a fresh checkout of the repo, ./scripts/setup.sh should finish without any errors. All other scripts are expected to be run from within Vagrant.

@kdeloach
Copy link

kdeloach commented Oct 6, 2016

+1

```bash
$ vagrant ssh
$ ./scripts/server.sh
$ ./scripts/server.ssh
Copy link

Choose a reason for hiding this comment

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

Typo on this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@rajadain rajadain force-pushed the feature/tt/initial-app-import branch from d07934a to c45cdd4 Compare October 6, 2016 19:31
@rajadain rajadain merged commit 6ba7a07 into develop Oct 6, 2016
@rajadain rajadain deleted the feature/tt/initial-app-import branch October 6, 2016 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants