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

New docs #236

Merged
merged 22 commits into from
Jan 9, 2023
Merged

New docs #236

merged 22 commits into from
Jan 9, 2023

Conversation

reubenharry
Copy link
Contributor

Mkdocs. Covers both docs and website. Looks nicer. Still on netlify.

@netlify
Copy link

netlify bot commented Dec 22, 2022

Deploy Preview for monad-bayes-old-docs canceled.

Name Link
🔨 Latest commit ed41148
🔍 Latest deploy log https://app.netlify.com/sites/monad-bayes-old-docs/deploys/63b489aaecd5ba000896d3cd

@reubenharry reubenharry requested a review from mknorps December 22, 2022 23:57
@netlify
Copy link

netlify bot commented Dec 22, 2022

Deploy Preview for monad-bayes-site failed.

Name Link
🔨 Latest commit ed41148
🔍 Latest deploy log https://app.netlify.com/sites/monad-bayes-site/deploys/63b489aaa3befc000945f700

@netlify
Copy link

netlify bot commented Dec 22, 2022

Deploy Preview for monad-bayes canceled.

Name Link
🔨 Latest commit 93c12f8
🔍 Latest deploy log https://app.netlify.com/sites/monad-bayes/deploys/63b58f70d69bc400087faecd

@reubenharry
Copy link
Contributor Author

@mknorps @idontgetoutmuch Here are what the new docs look like: https://monad-bayes.netlify.app/ The motivation was that currently I'm hosting two separate sites, one for the docs and one for the notebooks, and this both looks cleaner and is easier to maintain. I think this repo is set up such that I can't merge this in myself, so if one of you could, that would be great, and then I can delete the old websites.

Copy link
Contributor

@mknorps mknorps left a comment

Choose a reason for hiding this comment

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

The site looks elegant! It has better user experience and you modernized it well. I like it very much.

Apart from the comments within the code, I have few comments and suggested changes regarding the PR:

  • in docs/images plot.pgn and plot copy.pgn are duplicates. Remove the copy.
  • How does tintin work here? I see a .tintin.yml file but no reference to it in teh code and requirements.
  • Do we still need monad-bayes-site folder?
  • For the structure of the project it would be better if the code to generate documentation was in a separate folder.
  • is monad-bayes-site.cabal and stack.yaml within docs still needed?

on:
push:
branches:
- newdocs
Copy link
Contributor

Choose a reason for hiding this comment

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

Change trigger to push on master and add cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure how to add cache, but added master.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should work:

      - name: Setup caching
        uses: actions/cache@v3
        with:
          key: ${{ github.sha }}
          path: .cache

Check out: https://gaseri.org/en/blog/2022-11-01-publishing-material-for-mkdocs-website-to-github-pages-using-custom-actions-workflow/

.github/workflows/ci.yml Outdated Show resolved Hide resolved
mkdocs.yml Outdated Show resolved Hide resolved
mkdocs.yml Outdated Show resolved Hide resolved
runtime.txt Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
@reubenharry
Copy link
Contributor Author

For some reason, the --force and Python 3.8 were both necessary for CI and netlify to work respectively, but I changed the other things.

@mknorps
Copy link
Contributor

mknorps commented Dec 30, 2022

For some reason, the --force and Python 3.8 were both necessary for CI and netlify to work respectively, but I changed the other things.

That is inconvenient, but we can leave with this for now.

And what about:

Do we still need monad-bayes-site folder?
For the structure of the project it would be better if the code to generate documentation was in a separate folder.

Is also mathjax no longer needed?

@reubenharry
Copy link
Contributor Author

And what about:

Do we still need monad-bayes-site folder?

I thought I removed it. I don't see it in the repo locally.

For the structure of the project it would be better if the code to generate documentation was in a separate folder.

Now done.

Is also mathjax no longer needed?

Re-added.

Copy link
Contributor

@mknorps mknorps left a comment

Choose a reason for hiding this comment

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

Thank you! The code structure looks better :)
One last thing - remove mkdocs.yaml from the main directory.

If you want to experiment a bit with caching, as I suggested in this comment it would be great, but if you do not have time for this now, please create an issue for later not to forget about it.

@reubenharry
Copy link
Contributor Author

OK, removed, and issue created.

@reubenharry
Copy link
Contributor Author

Hm, netlify works fine, but something with CI fails. I'm a bit stumped. Any ideas?

@reubenharry
Copy link
Contributor Author

Oh right, it's failing for the old site. Let me remove it...

@reubenharry
Copy link
Contributor Author

Now passes CI, should be ready to merge.

Copy link
Contributor

@mknorps mknorps left a comment

Choose a reason for hiding this comment

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

It looks OK now. Thank you for improving the documentation!

I created #239 to take care of the developer's environment regarding developing documentation.

@reubenharry reubenharry merged commit 110e040 into master Jan 9, 2023
@reubenharry reubenharry deleted the newdocs branch January 9, 2023 15:47
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.

2 participants