Skip to content

Update dynamic theme fixes for various elements#14843

Open
mmacolley wants to merge 1 commit into
darkreader:mainfrom
mmacolley:patch-1
Open

Update dynamic theme fixes for various elements#14843
mmacolley wants to merge 1 commit into
darkreader:mainfrom
mmacolley:patch-1

Conversation

@mmacolley

Copy link
Copy Markdown

Added fix for LogicMonitor

@Myshor

Myshor commented Dec 11, 2025

Copy link
Copy Markdown
Collaborator

This PR in current form is rejected.

  1. Share screens what exactly you try to fix as I could not find any changes landing on https://www.logicmonitor.com/. Example URL would be the best if possible to share.
  2. Remove asterisk from URL in your fix *. or you are fixing some specific sites that have prefixes other than www for logicmonitor.com.
  3. There must be empty line before and after ================================. You do not have empty lines in the beginning and end of your sitefix.
  4. I saw you used static colors for fix. I bet it fixes something when Dark Reader's (default) dark scheme is used. However Dark Reader can be used with light scheme too and forcing colors which are fine in dark can totally break light scheming. In this case we try to use ${[color_to_be_used_in_light]}. For example to have rgb(112, 115, 117) in dark we put ${rgb(138, 139, 136)}. "Easy convert for rgb(r, g, b) used in dark would be => ${rgb(255-b, 255-g, 255-r)}.

Summarizing:
Even if you will make fixes in sitefix then we will need to confirm it will work fine in both dark and light scheme. That's why it's even better to share URLs or you need to share screens with:

  • Original (Dark Reader without fixes)
  • Dark Reader after fixes in dark scheme
  • Dark reader after fixes in light scheme

I did not put Review comments into code because it would be almost everything commented.
Setting Review result to "Request Changes".

@Myshor Myshor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See above.

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.

2 participants