-
Notifications
You must be signed in to change notification settings - Fork 178
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
Improve documentation about build process #238
Conversation
It looks good to me! Thanks for changing and for asking! :) |
build/README.md
Outdated
@@ -17,7 +17,8 @@ Please reference [Pull Request #40](https://github.com/manubot/rootstock/pull/40 | |||
|
|||
Note: currently, **Windows is not supported**. | |||
|
|||
Install or update the [conda](https://conda.io) environment specified in [`environment.yml`](environment.yml) by running: | |||
Install or update the [conda](https://conda.io) environment specified in [`environment.yml`](environment.yml) by running | |||
the following commands from this directory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably don't want this newline
build/README.md
Outdated
Otherwise, `build.sh` uses [WeasyPrint](https://weasyprint.org/) to build the PDF. | ||
It is common for WeasyPrint to generate many warnings and errors that can be safely ignored. | ||
Examples are shown below: | ||
```sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove sh
... this is not shell, just stdout to the shell.
README.md
Outdated
|
||
# Configure the webpage directory | ||
python build/webpage.py | ||
|
||
# View the manuscript locally at http://localhost:8000/ | ||
# View the manuscript webpage/index.html locally at http://localhost:8000/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably note that usually you should be able to just open webpage/index.html
in a browser and it will work. However, to open a local webserver use the following:
This has changed from before. The HTML outputs now work better when just openning in browser without web server.
WARNING: Ignored `font-display:auto` at 1:53114, descriptor not supported. | ||
ERROR: Failed to load font at "https://use.fontawesome.com/releases/v5.7.2/webfonts/fa-brands-400.eot#iefix" | ||
WARNING: Expected a media type, got only/**/screen | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should say something like:
Setting
BUILD_PDF=false
will disable all PDF creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the top of the file we have
However, setting the
BUILD_PDF
environment variable tofalse
will suppress PDF output.
For example, run local builds using the commandBUILD_PDF=false bash build/build.sh
.
Should we move that or leave it there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I missed that from the diff. Your call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave it for now. Feel free to move it later if we make another pass through the documentation.
This build is based on ed4158c. This commit was created by the following Travis CI build and job: https://travis-ci.com/manubot/rootstock/builds/117357232 https://travis-ci.com/manubot/rootstock/jobs/211967438 [ci skip] The full commit message that triggered this build is copied below: Improve documentation about build process (#238) * Improve documentation about build process * Additional rephrasing * Updates per dhimmel review
This build is based on ed4158c. This commit was created by the following Travis CI build and job: https://travis-ci.com/manubot/rootstock/builds/117357232 https://travis-ci.com/manubot/rootstock/jobs/211967438 [ci skip] The full commit message that triggered this build is copied below: Improve documentation about build process (#238) * Improve documentation about build process * Additional rephrasing * Updates per dhimmel review
Some of the documentation about the local build script had not been updated for a while. @sbonaretti would these updates help address the undocumented behavior that led to your questions in #237?
Closes #237