Skip to content

Conversation

@ljmnoonan
Copy link

@ljmnoonan ljmnoonan commented Oct 15, 2025

This could be 2 separate PRs, so please let me know if you would like me to split them.
The original purpose of this PR was to make this module play with web_dark_mode as promised in my previous PR, #3324. This is implemented in 5f2789c.

Here are some screenshots of this module in use in conjunction with web_dark_mode:
Before:
image

After:
image
image

However, while implementing the dark mode assets, I discovered a whole bunch of broken features and deficiencies in the existing code, so I decided to go about fixing them. All the changes are detailed in the commit message on a3e37ed, but I will attach some before and after pictures here for clarity.

Add $wtc-input-color-required

Before:
image

After:
image
image

Add CSS selectors to consistently handle many2many fields added as a custom property

Before:
image

After:
image
image

Fix monetary field selectors.

Before:
image
image

After:
image
image

Please let me know if you would like any changes!

@OCA-git-bot
Copy link
Contributor

Hi @legalsylvain,
some modules you are maintaining are being modified, check this out!

@ljmnoonan ljmnoonan changed the title [IMP] web_theme_classic Add dark mode and some cleanup [18.0][IMP] web_theme_classic Add dark mode and some cleanup Oct 15, 2025
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for this refactoring and improvment !

@remi-filament
Copy link
Contributor

Hi @ljmnoonan thanks for this PR, I started looking at how to improve borders on monetary fields and realized you did it as well !

Would you mind trying to also fix the currency symbol which is not aligned with amount :
image

While testing, I found out that adding

align-self: center;

does the trick (although Firefox reports that it has no effect)

@ljmnoonan ljmnoonan force-pushed the 18.0-web_theme_classic branch from f203db2 to bfe3fe8 Compare October 21, 2025 22:08
@ljmnoonan
Copy link
Author

@remi-filament, thank you very much for the heads up!
Even though Firefox doubts it, align-self: center; does seem to be the best fix for the the wonky placement of the monetary symbol.
While working on this I also changed the 2px margin to keep the symbol off the border to be on the symbol element instead of the input element. This allows me to account for which side it was on, although I must admit, the css selectors are a little crazy and will surely break if odoo ever changes anything, but at least it won't be very noticeable.
Also, I discovered that the searchbar in settings was getting a double border, so I fixed that.
The one remaining problem that I am noticing is that if you put invalid values into an input the background glows red as expected, but the border does not change to red. I do not think this can be realistically fixed though, so just leaving it.

Screenshots:

image image image image image

@ljmnoonan ljmnoonan force-pushed the 18.0-web_theme_classic branch from bfe3fe8 to acb7550 Compare October 21, 2025 22:29
Copy link
Contributor

@remi-filament remi-filament left a comment

Choose a reason for hiding this comment

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

Great, thanks @ljmnoonan it is much better and I think current limitations are accceptable (were already there before I guess) !
Maybe it is worth adding them in README in ROADMAP section ?

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@legalsylvain
Copy link
Contributor

Hi. ready to merge ?

regards

@ljmnoonan
Copy link
Author

@legalsylvain
Not yet, please. I am working on a solution for invalid fields. Should get it done today

Refactor all selectors to mirror the way odoo sets them. This results in a MUCH broader application of our styles that is not limited to form views.
Prefix all vars with "wtc-" in order to prevent them from overriding or being overridden by bootstrap vars.
Suffix all vars with !default to ensure that they can be overriden by dark mode.
Add $wtc-input-color-required to ensure text contrasts on $wtc-input-background-color-required. This is especially necessary in instances like the quantity and unit price columns of a sale order because by default these fields are set to a light blue.
Remove border on .note-editable, i.e. the HTML editor. This feature was broken and, as far as I am concerned, unnecessary.
Remove all button customization as it was all broken and is superceeded by OCA/web/web_save_discard_button anyways.
web_dark_mode is not merged yet.
OCA#3324
@ljmnoonan ljmnoonan force-pushed the 18.0-web_theme_classic branch from acb7550 to 5f2789c Compare October 26, 2025 11:18
@ljmnoonan
Copy link
Author

@legalsylvain @remi-filament
This should be ready to merge now. However, I would appreciate if you look it over again.
Long story short, while trying to get the "field invalid" styling to work I went down the rabbit hole figuring out how odoo does it. It turns out that they do not actually change the borders, but rather the color variable in css on the front end. We were using a sledgehammer by overriding it on the backend with a scss variable. Also, odoo was actually setting borders as transparent by default, but we were trying to hide them via border: 0; which caused the alignment issue that @remi-filament pointed out.

Having already wasted enough time on it, I went all in and adjusted the whole thing to follow odoo's example, and I believe the result is much more comprehensive.

Please let me know what you think!

PS: I will update the PR description and screenshots to reflect the new state of things in order to avoid confusion in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants