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

PR: Add support for docs versions #185

Merged
merged 33 commits into from
Aug 30, 2020
Merged

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Aug 25, 2020

Pull Request Checklist

  • Read and followed this repo's Contributing Guidelines
  • Based your PR on the latest version of the correct branch (master or 3.x)
  • Checked your writing carefully for correct English spelling, grammar, etc
  • Described your changes and the motivation for them below
  • Noted what issue(s) this pull request resolves, creating one if needed

Description of Changes

Currently only builds local branches that follow the regex in the config (branches with <number>.<anything> names and master).

NOTE: The theme being installed needs to be changed for the proper repo/branch after spyder-ide/spyder-docs-sphinx-theme#33 is merged

A preview:

versions

Depends on spyder-ide/spyder-docs-sphinx-theme#33

Issue(s) Resolved

Fixes #179

@dalthviz dalthviz self-assigned this Aug 25, 2020
@dalthviz dalthviz closed this Aug 25, 2020
@dalthviz dalthviz reopened this Aug 25, 2020
@ccordoba12
Copy link
Member

ccordoba12 commented Aug 26, 2020

@dalthviz, please don't forget to update the way our docs are installed in Netlify to check how your work looks there.

@CAM-Gerlach
Copy link
Member

@dalthviz I'm not sure what specifically @ccordoba12 is referring to, but Netlify needs to be set to clone all branches like you did for Travis or only the current branch will show up in the dropdown on Netlify, as you can see in the preview. Not sure how best to do this with Netlify, but I'm sure you do.

@ccordoba12
Copy link
Member

I'm not sure what specifically @ccordoba12 is referring to

To use make multidocs instead of make docs there.

but Netlify needs to be set to clone all branches like you did for Travis or only the current branch will show up in the dropdown on Netlify

I don't know if that's possible because Netlify is in charge of cloning the repo (users don't control part).

@dalthviz
Copy link
Member Author

dalthviz commented Aug 26, 2020

To use make multidocs instead of make docs there.

For that we merged #186 and rebased this (since other PRs like #181 need to use make docs instead of make multidocs to generate a preview at least until we merge this PR).

Netlify needs to be set to clone all branches

@CAM-Gerlach I was thinking about that too although, as @ccordoba12 says, I'm unsure if it's possible. I will need to check

@dalthviz
Copy link
Member Author

Seems like Netlify builds a detached HEAD of the PRs without remotes:

image

Also thinking a little bit more, maybe having all the versions on the preview isn't actually needed right @CAM-Gerlach @ccordoba12 ? At the end when you are doing a modification you target that to only one version (depending on the base branch the PR sets) and I think Netlify puts your changes and builds that specific branch/version

@ccordoba12
Copy link
Member

maybe having all the versions on the preview isn't actually needed right?

Sure, we can go without that for now.

@ccordoba12
Copy link
Member

ccordoba12 commented Aug 26, 2020

I have some comments about the style though:

imagen

  • I think we should style "Versions" in the same way as the rest of the text on the sidebar.
  • "Versions" should be aligned with the title of the page or be a little below it.
  • There's a margin on the left that prevents "Versions" and the dropdown to be aligned with "Search".
  • We should style the dropdown in the same way as "Search".

My proposal (this also changes the border of "Search" a bit):

imagen

#versions-header {
    color: rgba(0,0,0,.65);
    font-size: 0.8rem;
    text-transform: uppercase;
    margin-top: 1.1rem;
}
#select-versions {
    margin-bottom: 7px;
    border: 1px solid rgba(0, 0, 0, 0.15)
    font-size: 90%;
    color: rgba(0,0,0,.65);
    padding: 2px;
}
#select-versions, #select-versions option {
    width: 98%;
}
.form-control {
    box-shadow: none;
    background: transparent;
    border: 1px solid rgba(0, 0, 0, 0.15);
    height: 54px;
    font-size: 18px;
    font-weight: 300;
}

@dalthviz dalthviz closed this Aug 26, 2020
@dalthviz dalthviz reopened this Aug 26, 2020
@CAM-Gerlach
Copy link
Member

@ccordoba12 As I've mentioned previously (somewhere... :) ) I've been working on that for a while and will push my changes shortly. It ended up being way more complicated to both do that and also build the current feature/PR branch, because I was having to set the release in conf.py dynamically (in order to also rename the output directories accordingly) and couldn't figure out any way to propagate the right state or env variables all the way through to the sub-build to get it to work reliably cross-platform but I think I've got it solved now.

@CAM-Gerlach CAM-Gerlach marked this pull request as draft August 28, 2020 21:55
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Just a few smaller style things, and one code issue. Otherwise, this should be good to go on my end. Thanks!

Makefile Outdated Show resolved Hide resolved
doc/_static/custom_scripts.js Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
doc/_static/custom_scripts.js Outdated Show resolved Hide resolved
doc/_static/custom_scripts.js Outdated Show resolved Hide resolved
doc/_static/custom_scripts.js Outdated Show resolved Hide resolved
ci/install.sh Outdated
pip3 install git+https://github.com/spyder-ide/spyder-docs-sphinx-theme.git@develop_spyder
pip3 install sphinx doctr sphinx-panels sphinx-multiversion
# You can change the command above to use a different version of the theme (but return it to the correct one before merging)
pip3 install git+https://github.com/dalthviz/spyder-docs-sphinx-theme.git@sidebar_widgets
Copy link
Member

@CAM-Gerlach CAM-Gerlach Aug 29, 2020

Choose a reason for hiding this comment

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

⚠️ ⚠️ ⚠️ REMINDER: MERGE THEME PR AND CHANGE BEFORE MERGING THIS ONE!!! ⚠️ ⚠️ ⚠️

@CAM-Gerlach
Copy link
Member

@dalthviz , @juanis2112 noticed that in Safari, the selector functions, but displays the wrong branch (always the current one) once the page is loaded. In Chrome, we can both confirm that it starts off on the default selection on page load, but within a couple hundred ms is set to the correct one, and in FF it is always correct from when the page first starts to render, but in Safari it says set to the current branch permanently.

image

@dalthviz dalthviz closed this Aug 29, 2020
@dalthviz dalthviz reopened this Aug 29, 2020
Copy link
Contributor

@juanis2112 juanis2112 left a comment

Choose a reason for hiding this comment

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

Apart from the small blink that CAM commented, everything works fine for me. Thanks @dalthviz and @CAM-Gerlach!

@CAM-Gerlach
Copy link
Member

@juanis2112 I fixed the "blink" issue with the version selector, and made a few related behind the scenes improvements

@dalthviz A few last changes:

  • Make the setupVersionSelector() function fire when the DOM is ready, instead of only when all external resources are loaded, to ensure it triggers earlier to reduce the window in which it is non-functional for users on slow connections (and originally to reduce or eliminate the blink, but I fixed it entirely with the next change
  • Statically set the option that is selected via a check in the Jinja template, and eliminate the now-unnecessary JS that sets it, so that it is set correctly from the very beginning and avoid the need for the extra complexity, fragility and delay it introduces
  • Only run setupVersionSelector() if there is a version selector on the page (e.g. make docs; I didn't observe any console errors or bad behavior but better safe than sorry)
  • Minor style tweaks and kill blank spaces

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM @dalthviz , I think this one is ready and everyone has approved. Thanks so much for your work! Can you and @juanis2112 recheck my last changes to fix the "<blink>" issue, and if everything looks good to both of you, go ahead and merge after merging spyder-ide/spyder-docs-sphinx-theme#33 and unsetting the custom theme branch.

@CAM-Gerlach
Copy link
Member

@juanis2112 rechecked and it looks good, so @dalthviz you're free to go ahead with the above, thanks.

@CAM-Gerlach CAM-Gerlach marked this pull request as ready for review August 30, 2020 03:31
@dalthviz dalthviz merged commit 87917e7 into spyder-ide:master Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up UI, redirects and deployment for use with multiple versions of the docs
4 participants