-
Notifications
You must be signed in to change notification settings - Fork 6
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
Theme update #58
Theme update #58
Conversation
…est jupyter book)
for more information, see https://pre-commit.ci
…-theme into theme-update
steps: | ||
- uses: actions/checkout@v2 | ||
|
||
- name: Cancel previous runs | ||
uses: styfle/cancel-workflow-action@0.12.0 |
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.
This can be replaced by built in concurrency
- https://github.blog/changelog/2021-04-19-github-actions-limit-workflow-run-or-job-concurrency/
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.
Thanks a lot; wasn't aware of this key. Just went over its docs and found it a bit more complicated than what is currently here (e.g. defining groups, specifying if concurrency for PR numbers, etc.), though please feel free to push the change if you'd like that better (and maybe find it more useful than the one here).
If I were to implement it, should it look something like this:
concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true
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.
Sorry, I didn't see this outstanding question. Yes, the concurrency group just needs to be non-unique for the same PR e.g.
group: 'docs-${{ github.head_ref || github.run_id }}'
This ensures that if the run doesn't have a head_ref
(non-PRs), then it falls back to a unique identifier.
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.
No worries! FYI: The theme update is working now, and its proper functioning (with a few non-vital issues) has been confirmed through tests by a few team members. It is a bit time sensitive before our upcoming community event; hence, I'd like to get it merged and go forward to be used by our portal.
I've created a new issue though to address this suggestion.
Thanks very much for all your help and review!
An FYI: The Pythia portal nightly build with this updated theme built successfully; however, we will need to inspect its artifact and maybe perform additional tests to investigate if the results of the theme is good for actual deployment. |
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.
@erogluorhan This looks pretty reasonable to me and I'm glad that it is working. However it's possible there are things I don't understand well enough to catch. Let me know if you want to do a walk through of the changes.
Also I'm wondering if there are any lessons you learned while working on this that could go into some internal documentation for ease of future updates.
Hey @jukent , thanks for that! Yes, I can give you a walk through of the current state anytime. Even though I marked this as review-ready, it was more for bringing it to attention of everyone in the loop, and get some feedback/help from the more experienced (since I am not at all experienced on this work). The current state of it seems good about that we were able to get Sphinx, theme, and dependency packages up & running with their unpinned versions. However, there is still work to do on getting this new version of the theme to reflect all the features listed here. Currently, I am looking into |
FYI: This is where I am as of now. You can download it and check out |
…ures (#59) * Re-populate layout * Remove docs-navbar block from layout * Remove docs-sidebar block from layout * Remove topbar.html block from layout * Remove _templates/prev-next.html block from layout * Add super() call to content block in layout * Add super() call to content block in layout * Cut the block content * Add banner and shim back * Add super() call back * Include _templates/prev-next.html * Add shim back * Add more into layout * Remove block docs_sidebar * Remove include topbar.html * Convert _templates/ to components/ * Remove super() call * Convert components/ to _templates/components/ * Footer and prev-next from parent * prev-next from parent * More layout * More layout * Fix super.super() call in layout * More layout * docs_navbar * docs_navbar * docs_navbar * docs_navbar * docs_navbar * docs_navbar * footer and silenced elements * docs_navbar * docs_navbar * shim and banner * --breakpoint-md to --bs-breakpoint-md * --breakpoint-md to --bs-breakpoint-md * footer * remove footer and more * Add back footer * footer * footer * footer * footer * footer * docs_body * docs_main super call * footer * footer * bd-footer * bd-footer * bd-footer * bd-footer * remove footer * add back footer * bd-footer * bd-footer super * footer * footer * footer * footer * footer * footer * footer.html * footer.html * footer.html debug * footer.html debug * footer * footer * sections/footer * footer * footer * footer-logos.html * footer-info.html * revert footer-info.html footer-logos.html * footer.html * footer.html * footer.html * footer.html * footer.html * footer.html * footer * footer * footer * footer * footer * footer * footer * layout * css * css colors * css colors * css colors * layout banner * css colors * css colors * css colors * css colors * css colors * footer.html * css * css * css * css * css * scss * css * css * css * css and scss * sphinx basic scss * sphinx basic scss * sphinx basic scss * css colors * css colors * css colors * css colors * Update docs
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.
Steps taken to build. From Portal or Foundations repository:
Then from the sphinx-pythia-theme repository:
Then in Portal:
Or in Foundations:
|
How does it look now, @jukent? (screenshot is from my local but I can push it if it is liked) |
Yeah that looks great! If you push the commit I'll re-test my build. |
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.
Thanks Orhan for all of the hard work that went into this. These changes seem sensible and we've had several chances to look at the Html together. Thanks for updating the CSS to make the foundations navbar more legible.
LGTM! I think this issue is holding up another PR and plenty of other book dev work. Thanks again!
@erogluorhan and I are meeting to chat at 11AM CDT today (@jukent feel free to join as well!), but I agree that we're on the right track and it will be great to get this merged soon since it will resolve a lot of existing issues. Some things I notice, cookbook-related at least, include:
|
Hey @agoose77 please see this discussion about the theme update. Thanks for everything! |
This is now a working
sphinx-pythia-theme
update with the followings:sphinx_pythia_theme/layout.html
updated to keep our website's layout/style almost 100% unchanged by using the layout/style changes in the parentsphinx_pythia_theme
sphinx_pythia_theme/footer.html
(though with some updates in it) to create our custom Pythia footer using the parent'sfooter_start
onlysrc/scss/sphinx-pythia-theme.scss
updated (a lot of!important
used) to keep Pythia's custom colors and styles working with the changes in the parent themessphinx-book-theme>=1.0.0