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

Bootstrap 5 template #1287

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

Bootstrap 5 template #1287

wants to merge 17 commits into from

Conversation

elrido
Copy link
Contributor

@elrido elrido commented Apr 21, 2024

This PR fixes #728 and adds an new template called "bootstrap5" based on Bootstrap CSS 5.3.3.

Changes

  • ADDED: Optional Bootstrap CSS 5.3.3 based template, use configuration template = "bootstrap5" to switch to it (Upgrade Bootstrap to v4 or higher #728)
  • CHANGED: Set lang cookie with strict SameSite property

Note that in the process of implementing this, some changes to privatebin.js and it's template specific code became necessary. While I have re-tested key functionality with "bootstrap", "bootstrap5" and "page" templates, there is some risk in there that this may have introduced some new issues due to the complexity. An example was with the modals, which in bootstrap 3 & 5 work similarly and have the same API, but don't behave 100% the same.

I would like to not yet switch to this new template by default and rather soonish get this into a point release, so people can test it and provide us feedback, also regarding some of the layout choices. Personally, I'm not too fond of the new default colours and the "flat" look, but wanted to keep it at the defaults or same classes as we do on the current bootstrap 3 template as a starting point.

My suggested rough timeline on this:

  • get this change reviewed and merged
  • provide bootstrap5 as an optional template, but not the default one, with the next point release (1.7.2 or 1.8.0)
  • keep the demo site at bootstrap 3, but I will update one of my personal instances to bootstrap 5, so I too can get some real life user XP on it - hopefully some other instances will start to experiment using it as well
  • in the background, we can work on branches for the pelican (project site) & tera (directory) templates to switch to bootstrap 5 as well
  • after some time (6 months?) and if not many (new) issues with it remain unresolved, we can change the default to "bootstrap5" and release a new major version (2.0.0), as well as update the project, directory and demo sites to all use the new look

current status:
- renders without PHP errors & passes unit tests
- displays pastes
- responsive navbar
- right-to-left support
- auto dark mode with toggle

to be done:
- add "Dark Mode" to translation strings
- get expiration and format selections to work
- fix modals (password, QR-code, etc.)
- replace glyphicons with Bootstrap Icons (no longer included)
- test all the different settings and combinations
- check tab alignment in HTML source
current status:
- got expiration and format selections to work
- fixed modals (password, QR-code, etc.)
- replaced glyphicons with Bootstrap icons (needs CSP relaxation to work)
- tested the different settings and combinations
- got editor tabs to change active status

to be done:
- add "Dark Mode" to translation strings
- figure out how to change prettify theme when dark mode gets selected
- check tab alignment in HTML source
current status:
- made prettify theme work with dark mode

to be done:
- fix password modal display
- add "Dark Mode" to translation strings
- check tab alignment in HTML source
@elrido elrido requested a review from rugk April 21, 2024 10:07
@rugk
Copy link
Member

rugk commented Apr 22, 2024

Okay, the plan sounds good, but we should actually really aim for deleting the old bootstrap code, it's unmaintained etc.

Also, could you possibly add a screenshot to the PR so one can already get a glance on how it looks like?

And not to forget… thanks a lot! I imagine this has been much work! 👍 🚀

@rugk rugk linked an issue Apr 22, 2024 that may be closed by this pull request
Copy link
Member

@rugk rugk left a comment

Choose a reason for hiding this comment

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

From a design/QA perspective, did you check whether this works with the existing syntax highlighting stuff? (This likely is not being replaced, though we maybe should look into having some more dynamic one, because now #1196 is unevitable, if the browser decides the theme? This is kind of a UX/UI issue…)

Apart from that I am through, most important things are probably (potential) a10n issues, otherwise just code style stuff…

css/bootstrap5/privatebin.css Show resolved Hide resolved
css/bootstrap5/privatebin.css Outdated Show resolved Hide resolved
css/bootstrap5/privatebin.css Show resolved Hide resolved
js/dark-mode-switch.js Show resolved Hide resolved
js/privatebin.js Show resolved Hide resolved
tpl/bootstrap5.php Show resolved Hide resolved
tpl/bootstrap5.php Show resolved Hide resolved
tpl/bootstrap5.php Outdated Show resolved Hide resolved
tpl/bootstrap5.php Outdated Show resolved Hide resolved
lib/View.php Show resolved Hide resolved
Copy link
Member

@rugk rugk left a comment

Choose a reason for hiding this comment

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

pressed button too fast

Co-authored-by: rugk <rugk+git@posteo.de>
@elrido
Copy link
Contributor Author

elrido commented Apr 23, 2024

Ok, so I've got some homework to do next weekend.

[...] but we should actually really aim for deleting the old bootstrap code, it's unmaintained etc.

We can do, but lets aim for at least some transition period, where the old is available in parallel, should there still new issues crop up when it gets more widely used. So i.e. < 2.0 keep it as a non-default option, 2.0 ~ 2.2 or 2.3 make bootstrap 5 default, keep bootstrap, > 2.2 or 2.3 remove bootstrap.

We could consider removing the page template and the now broken zerobin legacy support (option in configuration file, legacy compression JS, etc.) with the 2.0 release. Though that goes offtopic.

Also, could you possibly add a screenshot to the PR so one can already get a glance on how it looks like?

Sure, you can find some at: #728 (comment)

did you check whether this works with the existing syntax highlighting stuff?

Yes, and that was actually some of the more tricky stuff. All the syntax highlighting related JS is in the dark mode JS, since I have to load a different CSS (= remove any existing theme, inject a new theme) every time the switch gets flipped.

For the existing templates these themes get loaded with the page and never change. Only the content in the relevant (hidden) preview gets updated from the textarea or loaded paste and an update of prettify gets triggered on it. Fortunately for this case, changing the CSS theme instantly changes the look of the already rendered preview.

elrido and others added 6 commits April 23, 2024 21:15
empty only works with variables, not constants - here we want to error out if PATH either isn't defined or does not end in a directory separator, so we can concatenate onto it
Co-authored-by: rugk <rugk+git@posteo.de>
@elrido
Copy link
Contributor Author

elrido commented May 1, 2024

Tried to answer/resolve all answers as best I could. Anything else, or Ok as is, can it get merged?

Copy link
Member

@rugk rugk left a comment

Choose a reason for hiding this comment

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

Replied there to some comments…

Apart from that already looks kinda good.

We should:

  • definitively rip out old template then, especially the page one, as you say… This also reduces the maintenance burden. There is no need to maintain a template nobody uses anymore anyway.
  • and yeah I agree that transition period makes totally sense
  • note some followup tasks to improve the code etc.

elrido and others added 2 commits May 2, 2024 08:01
Co-authored-by: rugk <rugk+git@posteo.de>
Co-authored-by: rugk <rugk+git@posteo.de>
Copy link
Member

@rugk rugk left a comment

Choose a reason for hiding this comment

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

Okay so only open thing is the header thing.

Apart from that I noted/collected the next iteration steps/stuff to do in #1298, which can best be tackled when the page template is gone.

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.

Upgrade Bootstrap to v4 or higher Automatic dark/light mode
2 participants