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

Adds guess-static-sites #27

Merged
merged 6 commits into from
May 7, 2018
Merged

Adds guess-static-sites #27

merged 6 commits into from
May 7, 2018

Conversation

khempenius
Copy link
Collaborator

No description provided.

Copy link
Contributor

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

Great work getting these changes in, Katie! I attempted to leave some review comments on 2f43f97, but got an error message from GitHub saying this commit was not part of the PR. It may be a temporary GH issue:

image

To avoid delaying you, some high-level comments:

  • "After downloading this repo, install the dependencies:" may be worth adding a one-liner before this to git clone https://github.com/guess-js/guess/ and cd experiments\guess-static-sites. Right now if a user clones the repo and runs npm install in root, they'll install our infra for other packages rather than what they intended :)

  • "This repo uses Google Analytics data to determine" - let's switch to "this directory" or "this folder" as it's now part of the main repository.

  • "A client-side script (which you'll add to your application) sends a request to the server to get the URL of the page it should fetch, it then prefetches this resource" I wonder if a casual reader might question what server we're informing them they'll be sending requests to. Perhaps we could expand on this to say "A client-side script (which you'll add to your application) sends a request to the server you are running to get the URL of the page it should fetch, it then.." (or something clearer indicating we aren't asking them to send our servers data).

  • package.json: I wonder if we should rename the name field to guess-static-sites-server? or something closer to the newer name now that we have one.

README.md Outdated
@@ -18,7 +18,7 @@ Should you wish to try out the modules we offer individually, the `packages` dir
***For non-Webpack users:***
### :black_circle: Data-driven loading

Our [predictive-fetching for sites](https://github.com/guess-js/guess/tree/predictiveFetching/predictiveFetching) workflow provides a set of steps you can follow to integrate predictive fetching using the Google Analytics API to your site.
Our [predictive-fetching for sites](https://github.com/guess-js/guess/tree/experiments/guess-static-sites) workflow provides a set of steps you can follow to integrate predictive fetching using the Google Analytics API to your site.
Copy link
Contributor

Choose a reason for hiding this comment

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

@khempenius
Copy link
Collaborator Author

Excellent suggestions! Fixed.

Copy link
Contributor

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

Thanks for the revisions! Landing this now 🚀

@addyosmani addyosmani merged commit 5328fbd into master May 7, 2018
@mgechev mgechev deleted the predictiveFetching branch May 7, 2018 14:58
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.

None yet

2 participants