Unify css with bootstrap themes #4745
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Bumps [bootstrap](https://github.com/twbs/bootstrap) and [@types/bootstrap](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/bootstrap). These dependencies needed to be updated together. Updates `bootstrap` from 5.2.0 to 5.3.0 - [Release notes](https://github.com/twbs/bootstrap/releases) - [Commits](twbs/bootstrap@v5.2.0...v5.3.0) Updates `@types/bootstrap` from 5.2.4 to 5.2.6 - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/bootstrap) --- updated-dependencies: - dependency-name: bootstrap dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: "@types/bootstrap" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
jorg-vr
added
the
enhancement
Co-authored-by: Charlotte Van Petegem <[email protected]>
bmesuere
requested changes
Jul 12, 2023
There was a problem hiding this comment.
👍 nice work. This removes a few scss hacks and unifies our variables to css.
Some remarks:
- I would not add a theme menu in the main navigation bar and instead make it a sub menu in the current user menu.
- The contrast of the selected option isn't great. The text should be white here (as is the case with the year selector on the signed in home page)
- I found it odd that the icon for auto displayed the current state. Maybe use https://pictogrammers.com/library/mdi/icon/theme-light-dark/ ?
- Is there a reason why the icons are inlined?
-
The sign in page looks bad in dark mode due to the icons. Should we disable dark mode here? - Why do we store theme in the session? (I know you didn't change this, just asking the question) I would make it entirely client side (and thus use local storage) or store it in the database as a user setting. If we store it in the database, we could also add the option to the profile page.
- The theme controller call should be excluded from being "stored". If you go to the sign in page, then switch themes and then sign in, you are redirected to /theme which doesn't exist.
bmesuere
approved these changes
Jul 28, 2023
chvp
approved these changes
Jul 31, 2023
This was referenced Jul 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request removes the separate dark mode css file. It is replaced by the new bootstrap 5.3 css theming.
A new js theme picker is also added, supporting, darkmode, lightmode and system mode. This more general approach will allow more themes in the future.
There were a lot of css variable to overwrite, more so because every component has some own css variable.
I have thus not overwritten every possible variable, but started with the obvious ones and then did some visuals checks while walking through the application. I then updated those colors which stood out as odd.
This means I might have missed some variables that should be overwritten because the relevant bootstrap class is only used on one specific page. Please let me know if you find such an occurrence.
I was also disappointed that I had to write my own
btn-dangerandbtn-successvariant classes as the bootstrap instances did not support theming well.Changes:
New theme selector:
The preferred theme is now stored for the user
Other pages should be unchanged.
Closes #4711 as all changes are contained here