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

Nr/enhancement switch to furo theme #67

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

RignonNoel
Copy link
Contributor

@RignonNoel RignonNoel commented Jul 26, 2022

I changed the theme to furo theme.

Since I was testing the result I also fixed multiple errors:

  • non-consecutive header level increase
  • reference target not found

Only 7 errors persists, if you have idea to solve it I'm fully open to it.

N.B: document isn't included in any toctree errors are already solved in PR #55

reference target not found

For these one, I just didn't found how to fix the route. So we can discuss it together to find a new route or maybe just remove them since they don't work actually:

/mnt/c/Users/noelr/WebstormProjects/intranet.neuro.polymtl.ca/computing-resources/neuropoly/gpus.md:19: WARNING: 'myst' reference target not found: ./#connect
/mnt/c/Users/noelr/WebstormProjects/intranet.neuro.polymtl.ca/geek-tips/git-annex.md:122: WARNING: 'myst' reference target not found: ./internal-server.md#new-repository
/mnt/c/Users/noelr/WebstormProjects/intranet.neuro.polymtl.ca/mri-scanning/mni-mcgill-7t-terra.md:15: WARNING: 'myst' reference target not found: ./

Disallowed nested header found, converting to rubric [myst.nested_header]

Even after some research I don't know how to fix these errors since it come from an "optional path" somebody tried to design to make the page easy to read.

/mnt/c/Users/noelr/WebstormProjects/intranet.neuro.polymtl.ca/courses.md:10: WARNING: Disallowed nested header found, converting to rubric [myst.nested_header]
/mnt/c/Users/noelr/WebstormProjects/intranet.neuro.polymtl.ca/courses.md:14: WARNING: Disallowed nested header found, converting to rubric [myst.nested_header]
/mnt/c/Users/noelr/WebstormProjects/intranet.neuro.polymtl.ca/courses.md:28: WARNING: Disallowed nested header found, converting to rubric [myst.nested_header]
/mnt/c/Users/noelr/WebstormProjects/intranet.neuro.polymtl.ca/courses.md:32: WARNING: Disallowed nested header found, converting to rubric [myst.nested_header]

fix #22
fix #58

@RignonNoel RignonNoel requested a review from kousu July 26, 2022 02:13
@jcohenadad
Copy link
Member

@RignonNoel i'd like to review before merging

is is possible to see the built pages online or do i have to built it locally? i've observed that local build is slightly different than the online rendered pages

@RignonNoel
Copy link
Contributor Author

is is possible to see the built pages online or do i have to built it locally? i've observed that local build is slightly different than the online rendered pages

It's a great question! It would be easy to deploy somewhere but I don't know if we have any ressources in the lab to do that since opening port is always complex with university security.

If you are ok, since it's an open source project I can deploy it on my own on one of my server, just the time to test. Does it feel right for you ? @jcohenadad. It would take only some minutes to do so.

@jcohenadad
Copy link
Member

is is possible to see the built pages online or do i have to built it locally? i've observed that local build is slightly different than the online rendered pages

It's a great question! It would be easy to deploy somewhere but I don't know if we have any ressources in the lab to do that since opening port is always complex with university security.

If you are ok, since it's an open source project I can deploy it on my own on one of my server, just the time to test. Does it feel right for you ? @jcohenadad. It would take only some minutes to do so.

let’s not overdo it i’ll build locally

@RignonNoel
Copy link
Contributor Author

let’s not overdo it i’ll build locally

No problem, if change you change your mind do no hesitate, it only take some minutes for me to deploy on one of my servers.

In the meantine I wait for an approval

@jcohenadad jcohenadad self-requested a review July 26, 2022 13:49
@jcohenadad
Copy link
Member

is this branch up to date with master? the software page looks older:

your branch:
Screen Shot 2022-07-26 at 10 01 32 AM

master:
Screen Shot 2022-07-26 at 10 01 35 AM

P.S. In fact, I do prefer the look of the 'older' page because the gallery takes the whole space to the right. Is that something that could be fixed?

jcohenadad
jcohenadad previously approved these changes Jul 26, 2022
Copy link
Member

@jcohenadad jcohenadad 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 1 minute ago i like this new theme 😊 thank you for the implementation @RignonNoel

@RignonNoel
Copy link
Contributor Author

@jcohenadad I think you mixed up the things a bit 😉

We are speaking about the intranet and not about neuropoly website.

@RignonNoel RignonNoel dismissed jcohenadad’s stale review July 26, 2022 14:40

Error on which project to test

@jcohenadad
Copy link
Member

We are speaking about the intranet and not about neuropoly website.

oh gosh! sorry about that-- looking now

@jcohenadad
Copy link
Member

jcohenadad commented Jul 26, 2022

what's the cleanest way to install/test?

julien-macbook:~/code/intranet.neuro.polymtl.ca $ make clean html 
Removing everything under '_build'...
Running Sphinx v4.2.0

Theme error:
no theme named 'furo' found (missing theme.conf?)
make: *** [html] Error 2

should we have a theme.conf as suggested?

@jcohenadad
Copy link
Member

jcohenadad commented Jul 26, 2022

ok i did this:

pip install .[sphinx]

if that's the right thing to do can someone pls update https://github.com/neuropoly/intranet.neuro.polymtl.ca/wiki

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

looks good! a few things that i preferred before, but it is minor and hopefully there are more pros than cons in moving to furo. Examples of cons:

  • hiding the left TOC not there anymore
  • the "previous/next" are at the very bottom-- less convenient
  • i'm not a fan of the blue fonts in the left TOC. i know it could be changed though...

@RignonNoel
Copy link
Contributor Author

RignonNoel commented Jul 26, 2022

ok i did this:

pip install .[sphinx]

if that's the right thing to do can someone pls update https://github.com/neuropoly/intranet.neuro.polymtl.ca/wiki

It's wrote at the top of the setup.py but i'm okay with you that it's really not that easy to find out. Unfortunatly README.md is used by Sphinx since the project has been put at the root and not in a docs/ folder.

  • I will update the wiki

ooks good! a few things that i preferred before, but it is minor and hopefully there are more pros than cons in moving to furo. Examples of cons:

hiding the left TOC not there anymore
the "previous/next" are at the very bottom-- less convenient
i'm not a fan of the blue fonts in the left TOC. i know it could be changed though...

I will try to see if I can adapt it a bit before merging. Will keep you update here if I found something or not

@jcohenadad
Copy link
Member

Unfortunatly README.md is used by Sphinx since the project has been put at the root and not in a docs/ folder.

that could still be fixed, right?

@RignonNoel
Copy link
Contributor Author

Unfortunatly README.md is used by Sphinx since the project has been put at the root and not in a docs/ folder.

that could still be fixed, right?

Yes, we just need to move all the content inside a folder and modify a bit the github actions that build and deploy the docs. But it's a really commons case.

If you want I can do that in a next PR to make the project more easy to rampup.

@kousu
Copy link
Member

kousu commented Jul 26, 2022

Thanks for doing this @RignonNoel! The new theme will be way less buggy going forward.

Unfortunatly README.md is used by Sphinx since the project has been put at the root and not in a docs/ folder.

that could still be fixed, right?

Yes, we just need to move all the content inside a folder and modify a bit the github actions that build and deploy the docs. But it's a really commons case.

If you want I can do that in a next PR to make the project more easy to rampup.

But please take a moment and think about if this is a good idea. The current setup means that https://github.com/neuropoly/intranet.neuro.polymtl.ca/ doubles as a copy of the wiki. If you put it in a subfolder people will have to know to navigate to https://github.com/neuropoly/intranet.neuro.polymtl.ca/tree/master/docs which I think is a really bad experience.

What if we just put a comment (a literal HTML comment) into README.md that said

<!-- look in setup.py for local build instructions -->

Also if that's not obvious enough, people can work it out by reading .github/workflows/, which are linked from the green Actions dots that appear next to every commit.

I think the knowledge about how to build locally is obvious enough for the small group of people who might be doing that, a much smaller group than those who might click through from https://github.com/neuropoly to https://github.com/neuropoly/intranet.neuro.polymtl.ca/ and be curious about reading our docs.

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

few issues-- see my comments

@jcohenadad
Copy link
Member

master:
image

branch:
image

@jcohenadad
Copy link
Member

lost space:
Screen Shot 2022-07-26 at 1 57 15 PM

@jcohenadad
Copy link
Member

another cons is the esthetic of the theme-- see notably how tables are rendered:

branch:
image

master (less messy):
image

@jcohenadad
Copy link
Member

in light of all the cons i've raised above, i'd like to reconsider moving to furo-- what are the pros again?

in the PR body, it says it fixes #22, but #22 is in fact already solved (see the issue)

and #58 is not the end of the world (ie: the cons of furo are more problematic to me).

@RignonNoel
Copy link
Contributor Author

RignonNoel commented Jul 26, 2022

From what @kousu explained, Furo is more easy to work with, due to code quality of the theme. Also it will allow us to have only one theme for the different internal project and so we will develop more skills around it (for example I already found different improvement we could do on SCT to get 100% width for content and fix colors problems)

@jcohenadad Here are some improvements I did:

  • Left space is now removed, I also changed the others part of the website to take 100% of the width
  • I found an extension to support tab as you found out for MsC/PhD, it solved
  • Did some enhancement/fix on color theme to have dark mode and light mode fully working

I can take time to check the tables if you want, but I will wait your both opinions @jcohenadad and @kousu before putting more time on it.


Here is a screenshot for example

Firefox_Screenshot_2022-07-26T20-47-39 584Z

@jcohenadad
Copy link
Member

i tried building and still see the issue with the tab:
image

but anyway, there are other cosmetic issues that remain. There are more urgent things to work on so i suggest to put a hold on this for now

@RignonNoel
Copy link
Contributor Author

RignonNoel commented Jul 27, 2022

i tried building and still see the issue with the tab

There are some new package to install to support tab, I think you just forgot to do the install command @jcohenadad:

pip install .[sphinx]

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.

Right menu does not display the current headings Consider furo theme
3 participants