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

Update docs_theme to boostrap V5.x #9404

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

Conversation

mostafaei2002
Copy link

I am trying to update the docs_theme to bootstrap v5.x, There were some problems with the previous version.
The process shouldn't be very hard since the whole HTML structure is less than 250 lines. I don't know how much custom css, js is written for the current active version though.
If everything goes smoothly I should finish in a week or two.
Ideally I would love to use something other than mkdocs to generate the static HTML web pages. Bootstrap itself seems fine but mkdocs isn't that good. I like something like Astro much better.
Probably better to keep it quick and simple though and not overdo it.

@mostafaei2002 mostafaei2002 marked this pull request as draft May 12, 2024 18:45
@mostafaei2002
Copy link
Author

Some updates on my progress so far. I setup webpack for the project and the development environment. The navbar structure is almost completely updated. The side nav structure can be a bit improved and I haven't really touched the main content so far.
For the next step there is some discussion needed as most of the project will be finished and the only thing to work on will be the styles, To be more precise customizing the bootstrap variables and styles it is. And also setting up the dark mode which should be very easy with the tools bootstrap provides.

@mostafaei2002 mostafaei2002 marked this pull request as ready for review May 19, 2024 15:28
@mostafaei2002
Copy link
Author

I would love to have hear your opinions on the new theme. There are some problems specifically with the old django rest framework picture and the sponsers' picture on the dark mode but everything else looks quite good to me.
It's fully responsive as far as I've tested it and there is light & dark themes. To build the website use the following commands.

  1. npm install inside /docs_theme
  2. npm run build:watch inside /docs_theme
  3. mkdocs serve inside / directory (don't forget to install mkdocs from requirements.txt beforehand)

@mostafaei2002
Copy link
Author

@cclauss It's been a while since my last commit. I would appreciate it if you could take some time to review the current state of the project. Thank you.

Comment on lines +7 to +9
"build": "webpack build",
"build:production": "webpack build --mode=production",
"build:watch": "webpack build --watch"
Copy link
Contributor

Choose a reason for hiding this comment

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

When I try to build the docs on master, I run mkdocs build and then I can just open the site/index.html file and it just works.

With this branch, the html file is missing all CSS styles, until I run npm i && npm run build, which seems like an extra step needed to deploy the docs. I have no idea how the actual docs are currently deployed nor if the project is willing to change that, but that might be a deal breaker.

Copy link
Author

@mostafaei2002 mostafaei2002 Jul 23, 2024

Choose a reason for hiding this comment

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

Instead of downloading the css files from bootstrap directly I've imported the needed parts of bootstrap manually which decreases the final bundle size and improves performance. We could build the final css files locally and commit them to the repo or create a workflow to automatically build the final css files both solutions work. In my opinion the second solution is better and cleaner. There's gonna be 10K of raw bootstrap code which is just unnecessary in my opinion.

@browniebroke
Copy link
Contributor

I deployed a preview of the new theme: https://669e11a0d553a7007d5ec657--effulgent-entremet-d2197e.netlify.app

@mostafaei2002
Copy link
Author

I deployed a preview of the new theme: https://669e11a0d553a7007d5ec657--effulgent-entremet-d2197e.netlify.app

Thanks for deploying it. The code formatting with prettify is not working in this deployed version for some reason though. It works fine on my local development server.

@browniebroke
Copy link
Contributor

The code formatting with prettify is not working in this deployed version for some reason though

I see, the request to fetch prettify-1.0.js returns a 404

@mostafaei2002
Copy link
Author

mostafaei2002 commented Aug 4, 2024

That's my bad actually, I accidentally added the dist folder into .gitignore which includes prettify-1.0.js, I'll fix that soon.
But as for the other issue, Committing the final build files adds +10k lines of unnecessary code to the repo. Updating the CI/CD pipeline should fix the issue but I am not sure if the workflow in main.yml is responsible for deploying the docs_theme or how the CI/CD pipeline in general is configured.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you share screenshots of the updates please?

@mostafaei2002
Copy link
Author

Full screen light mode

image

Full screen dark mode

image

@mostafaei2002
Copy link
Author

The theme is also responsive, I could share the tablet/mobile views if you want.
The only remaining issue is deployment, If there is a CI/CD pipeline which ideally there should be there are 2 solutions:

  1. Add the final css/js build files into the repository: which adds a lot of useless bootstrap code to the repository which should be reviewed every time for security reasons.
  2. Modify the CI/CD pipeline: Add a step to install the dependencies with npm install and another to build the final bootstrap/js files with npm run build:production

If there are any other issues which need to be fixed i would be happy to know.

@auvipy auvipy requested review from a team and removed request for cclauss September 7, 2024 11:31
Copy link
Contributor

@peterthomassen peterthomassen left a comment

Choose a reason for hiding this comment

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

The code itself lgtm, and I think this is an improvement.

I see two outstanding questions, both to be answered by @tomchristie:

  • Are you OK adding npm install/build to the pipeline?
  • Are you OK with how the sponsors' logos are handled? (The previous scheme showed a random pick on the left; that seem to no longer work.)

@tomchristie
Copy link
Member

Thanks @mostafaei2002, @peterthomassen

I’m going to be slightly awkward and blocking here.

I don’t think this ties in neatly with getting a unified doc style throughout encode, and so I’m going to suggest we hold off on this.

Are you OK adding npm install/build to the pipeline?

Not really, no.

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.

6 participants