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

Themeing support #87

Merged
merged 5 commits into from
Jul 28, 2022
Merged

Themeing support #87

merged 5 commits into from
Jul 28, 2022

Conversation

karthik2804
Copy link
Contributor

Added ability to add themes by pulling in git submodules to a themes/ folder as per discussion on #73. The themes can be configured using config/site.toml. Added documentation as well but would definitely need to be proofread.

Any thoughts/suggestions would be appreciated.

Signed-off-by: karthik2803 <raams.karthik@gmail.com>
Signed-off-by: karthik2803 <raams.karthik@gmail.com>
@radu-matei
Copy link
Member

Besides a sign-off, could you please also sign your commits, please?
https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

Thanks!

@radu-matei
Copy link
Member

Quick question: why are the static files from the docs directory moved to docs/themes/?
I would expect that, if we refactor the current templates into an actual theme, that would be a separate PR.

@karthik2804
Copy link
Contributor Author

They were just moved there to document and test it as a theme. I could move them back to the original location along with the failing test.

@radu-matei
Copy link
Member

In general, we want to change as few files as necessary as part of a PR.

Thanks!

Signed-off-by: Karthik Ganeshram <kganeshram@ucdavis.edu>
@technosophos
Copy link
Collaborator

This is super cool!

docs/content/themes.md Outdated Show resolved Hide resolved
docs/content/themes.md Outdated Show resolved Hide resolved
Signed-off-by: Karthik Ganeshram <kganeshram@ucdavis.edu>
@karthik2804
Copy link
Contributor Author

I was also wondering which section the documentation will follow in the existing documentation so that I can add the proper links to the site to maintain the chaining of secitons.

Copy link
Member

@flynnduism flynnduism left a comment

Choose a reason for hiding this comment

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

Thanks @karthik2804 this is a great contribution to the project! ❤️

Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

This looks great. Just a handful of typo/grammar suggestions. Looks like the bartholomew.wasm file needs to be updated as well after the recent merge. Thank you!

docs/content/themes.md Outdated Show resolved Hide resolved
docs/content/themes.md Outdated Show resolved Hide resolved
docs/content/themes.md Outdated Show resolved Hide resolved
docs/content/themes.md Outdated Show resolved Hide resolved
docs/content/themes.md Outdated Show resolved Hide resolved
docs/content/themes.md Outdated Show resolved Hide resolved
docs/content/themes.md Outdated Show resolved Hide resolved
@karthik2804
Copy link
Contributor Author

karthik2804 commented Jul 28, 2022

@vdice I will resolve the conflict along with the fixes and push a new commit. I had one question as to where the docs for themes must be listed.

The current order of sections is

  1. Introduction
  2. Quickstart
  3. Templates
  4. Configuration
  5. Scripting
  6. Markdown guide

I am not sure where the "themes" section fits in that order.

@vdice
Copy link
Member

vdice commented Jul 28, 2022

@karthik2804 good question. No strong opinion here; I think the current ordering in this PR of 4. Configuration 5. Themes 6. Scripting (and so on) makes sense. We'll want to update the end sections accordingly, eg update https://github.com/fermyon/bartholomew/blob/main/docs/content/configuration.md?plain=1#L40 to navigate to the new Themes section and then at the end of the Themes doc we can link to the Scripting page.

Signed-off-by: Karthik Ganeshram <kganeshram@ucdavis.edu>
Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

LGTM!

@vdice vdice merged commit dca3741 into fermyon:main Jul 28, 2022
@karthik2804 karthik2804 deleted the themeing_support branch July 28, 2022 21:39
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.

5 participants