-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[18.0][IMP] web_theme_classic Add ability to toggle theme #3394
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
base: 18.0
Are you sure you want to change the base?
Conversation
|
Hi @legalsylvain, |
|
Closes #3234 |
6122b17 to
a9eb091
Compare
legalsylvain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature looks disabled by défaut. Or did i missed something ?
|
@legalsylvain yes, I left the persistent option disabled by default so that the toggle on the user menu would be visible and individual users can make a choice whether to turn it on or not, and what mode to use. |
Well, If someone install this module, he hopes that the feature will be enabled by default. Otherwise, it forces each user to enable it manually. So, yes, please put the value enabled by default. If user want to disable it, he can. |
|
@legalsylvain Yes, I see what you mean. I will change it |
a9eb091 to
993fb6c
Compare
|
@legalsylvain All set! |
legalsylvain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks !
6326de0 to
7681743
Compare
|
While working in I also updated the tests to HttpCase, which makes a lot more sense in this context |
Now you can either enable by user with Classic Theme Persistent setting, or toggle it on and off like dark mode. It uses a cookie to remember your preference between reloads.
7681743 to
3cfbe2e
Compare
thanks. good to know. |
|
@jappi00 if you get some time, would you mind reviewing this PR? |
|
@ljmnoonan functional test looks good. Will look into the code now. |
jappi00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Two little nitpicks 💃
| This module allows each user to choose whether they would like input fields to be displayed the "classic" way or the new, standard way (as if this module were not installed) | ||
|
|
||
| To do this you can either: | ||
| + Check "Classic Theme Persistent" in user preferences. This will enable the classic theme for that user across all devices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should write "Uncheck" since you enabled the theme by default if the conversation in the pr is right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ofc: nitpick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should stay "check" as writing "uncheck" would necessitate inverting the whole sentence. It's kind of odd describing a feature by what it doesn't do when it is turned off. The next line describes behavior when unchecked too.
|
|
||
| # These fields should be here in order to be accessible via in js | ||
| # as user.settings.persistent_classic_theme | ||
| persistent_classic_theme = fields.Boolean(default=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: maybe add help for the note from the configure.md, something like: "When switching the theme all users need to reload the page to have a effect."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a pretty verbose tooltip defined in res_users.py, although it does not mention the need to reload. I'm hesitant to add this though as pressing "Save" from the view_users_form_simple_modif modal actually triggers a reload, so it is only in the context of view_users_form that this is relevant. Also, I think it is something that should be rather obvious to any user savvy enough to enable debug mode, which is necessary to see this option in the context of view_users_form in the first place.
If @legalsylvain thinks it is necessary too, I will add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljmnoonan oh, clear!
|
This PR has the |
|
@jappi00 Thank you for the review! |
Now you can either enable by user with Classic Theme Persistent setting, or toggle it on and off like dark mode. It uses a cookie to remember your preference between reloads.
Screenshots
Toggle Hidden because Classic Theme Persistent turned on