Skip to content

Conversation

drammock
Copy link
Member

@drammock drammock commented Feb 16, 2021

  • lets the versionwarning.js script also generate the "unstable" warning bar on the development version
  • links to the corresponding page (if it exists) in the current stable (or dev) docs, instead of always linking to the homepage.

drawbacks:

  • not thoroughly tested yet
  • the bootstrap 4 classes aren't going to exist on old versions of the docs, so the styling will be messed up.

I'm opening this mostly for discussion purposes, so y'all can see what this solution would look like. I think the actual right way to fix this is:

  1. minimally update versionwarning.js to do the linking to the corresponding page (but keep the old devbar_style stuff)
  2. put a similar javascript into the main codebase (i.e., not in the root of mne.tools) that uses BS4 classes instead of hardcoded styles
  3. in current main, remove the part of doc/_templates/layout.html that inserts the devbar, and update it to load a new version of this versionwarning function that lives in doc/_static/ somewhere.

That way, old docs use the old script here (which has appropriate styling) and new docs will use the new function (which has appropriate classes for the new theme), and will continue to work well once they become no longer the current dev version.

@drammock
Copy link
Member Author

cc @larsoner @cbrnr

@larsoner
Copy link
Member

The only disadvantage I see to the separate JS version idea is that in the future if we want to update the JS redirect code, we not only have to do it in two places (main and here), but also in the JS code of the N releases that occurred between now and when we make that update because those will each have their own JS code. But it does allow styling to work properly, so I can live with this.

@drammock
Copy link
Member Author

Hmm good point. Now that I think about it, we could actually stick with just this file and assign the proper class conditionally based on the value of version (BS3 classes for old docs, BS4 for current and newer docs).

@cbrnr
Copy link

cbrnr commented Feb 17, 2021

If this works @drammock I think this would be the best solution. We want to be prepared to support different styles per version and not forced to stick with stuff that we needed 10 versions ago.

@larsoner
Copy link
Member

+1 for styling based on version >= 0.23 or not

@drammock drammock marked this pull request as ready for review February 17, 2021 16:59
@hoechenberger
Copy link
Member

Thanks for the work, @drammock!

Two suggestions to improve a11y, after a discussion with @opyh

  • turn the entire sentence Switch to the ... into a link
  • even better for mobile: the link should be a button element -> easier to hit with the fingertip

@drammock
Copy link
Member Author

  • turn the entire sentence Switch to the ... into a link

That is not feasible since the sentence sometimes has 2 links in it: This is documentation for an old version (0.21) of MNE-Python. Switch to the stable version, or the (unstable) development version. where "stable version" and "development version" are 2 different links.

  • even better for mobile: the link should be a button element -> easier to hit with the fingertip

Here's how that looks (spacing is pretty badly compromised):

Screenshot_2021-02-17_11-11-41

@cbrnr
Copy link

cbrnr commented Feb 17, 2021

I guess we can't try it on CircleCI because it depends on the URL being mne.tools? I actually don't like how it looks with buttons, and I'm also fine with linking "stable version" and "development version". However, you could remove the comma before "or", right?

@drammock
Copy link
Member Author

I've been testing it on the live site by going to old version of docs, using browser inspector to delete the existing node, then copy/pasting this script into the console to re-generate the node based on the new code. I've found a button-based version that I'm OK with, will push another commit.

@drammock
Copy link
Member Author

OK, buttons look terrible on the new site:

dev-version

@hoechenberger
Copy link
Member

OK, buttons look terrible on the new site:

😅

I can have a look later tonight to experiment a little, if that's alright with you? Or are you in a hurry to get this merged?

@drammock
Copy link
Member Author

I'm happy to have you take over @hoechenberger. I actually am about to be off work until friday morning. I worked around the horizontal spacing issue by making them each separate clauses, but using buttons affects vertical alignment of the text too (even with btn-link class, where it gets styled as a normal link), so now the baselines don't match. there is also a btn-sm class in both BS3 and BS4 but then the font sizes are different... If you can find a way to use buttons and have it look good, go for it. Otherwise feel free to remove the button classes.

FYI, when this goes in, there should be a corresponding PR to mne-python that removes the devbar block from doc/_templates/layout.html: https://github.com/mne-tools/mne-python/blob/dfc05947ebdd047aa078776328ae456e9d0fba77/doc/_templates/layout.html#L21-L32

@hoechenberger
Copy link
Member

Thanks @drammock, let's see how far I'll get until Friday, so we can maybe make a final decision during or after the Office Hour :)

@hoechenberger
Copy link
Member

hoechenberger commented Feb 19, 2021

This is what I've got so far, I think it looks quite pleasant:

Wide screen:
Screen Shot 2021-02-19 at 13 12 05

Narrower screen:
Screen Shot 2021-02-19 at 13 09 01

Even narrower:
Screen Shot 2021-02-19 at 13 08 52

WDYT?

@cbrnr
Copy link

cbrnr commented Feb 19, 2021

Looks nice! Can you show how it looks with two buttons when you are on the site of an old version? Then it shows links to stable and unstable. IMO, we could just show a link to stable if two buttons don't work.

@hoechenberger
Copy link
Member

Looks nice! Can you show how it looks with two buttons when you are on the site of an old version? Then it shows links to stable and unstable. IMO, we could just show a link to stable if two buttons don't work.

Oh, thanks for asking, I hadn't thought of that. I'm currently AFK, will look into this once I get back home!

@drammock
Copy link
Member Author

Per discussion during office hours, @hoechenberger will edit the JS to have the button, using BS4 classes btn btn-danger my-2 ml-4 font-weight-bold or similar, and will remove the second button that shows up on the old docs pages (i.e., old docs will only have link to stable, not also dev version).

@hoechenberger
Copy link
Member

hoechenberger commented Feb 19, 2021

I managed to resolve the issue with the unpleasant vertical misalignment between warning message and button text. (it's still not 100% perfect but I think it's good!)

With 95ad385 it would look like this:

Current dev

Screen Shot 2021-02-19 at 20 52 53

Viewing 0.23 docs after 0.24 has been released

Screen Shot 2021-02-19 at 20 52 40

Viewing docs of versions <0.23 (constraining us to Bootstrap 3)

Screen Shot 2021-02-19 at 20 53 58

- removed `d-md` and `text-dark` (not clear why needed?)
- removed `alert-link` (not needed for buttons)
- simplified margin CSS on BS3 version
- fixed: no `font-weight-*` classes in BS3
- refactored single-use constant "style"
@hoechenberger
Copy link
Member

Nice, thanks @drammock!

Co-authored-by: Richard Höchenberger <[email protected]>
hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Feb 20, 2021
@drammock drammock merged commit b6eb330 into main Feb 21, 2021
drammock pushed a commit to mne-tools/mne-python that referenced this pull request Feb 21, 2021
@drammock
Copy link
Member Author

thanks @hoechenberger !

@drammock drammock deleted the update-versionwarning branch February 21, 2021 20:53
@hoechenberger
Copy link
Member

Thanks for your help & support, @drammock, I appreciate it

@cbrnr
Copy link

cbrnr commented Feb 22, 2021

This is such a useful change! Best feature ever! Now I can quickly switch from dev to stable! 🚀 🔥 💯

@larsoner
Copy link
Member

As mentioned in mne-tools/mne-python#8876 (comment) there is a problem with unchecked linking, I just for example tried to get to stable for this:

https://mne.tools/0.19/generated/mne.preprocessing.mark_flat.html

@hoechenberger do you want to try https://stackoverflow.com/questions/3922989/how-to-check-if-page-exists-using-javascript#answer-3924998 ?

@hoechenberger
Copy link
Member

hoechenberger commented Feb 22, 2021

I can have a look later today. Wanted to address some a11y things first

@hoechenberger
Copy link
Member

hoechenberger commented Feb 22, 2021

What do you think should happen if the page doesn't exist? Link to root/index.html of stable?

@larsoner
Copy link
Member

Yes

@hoechenberger
Copy link
Member

You think this could be sth for the sprint?

@larsoner
Copy link
Member

I wouldn't make people mess with this repo for the sprint, I think it's easier just to stick with mne-python changes there

@hoechenberger
Copy link
Member

Good point.

@drammock
Copy link
Member Author

Dang it, I thought this was already handled. Either I'm bad at JavaScript or bad at testing 🙁

@larsoner
Copy link
Member

You're already ahead of me if that's an exclusive "or" :)

@drammock
Copy link
Member Author

OK, appears to be fixed now by #24

marsipu pushed a commit to marsipu/mne-python that referenced this pull request Feb 25, 2021
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.

4 participants