Skip to content

Conversation

@Sooperfish
Copy link

@Sooperfish Sooperfish commented Jan 3, 2025

genius.com https://genius.com/ (sitewide)

  • Header: Color, logo and signup link fix

nytimes.com

  • Games hub https://www.nytimes.com/crosswords
    • NYT Games logo, hamburger menu and progress icons fix
    • Letter Boxed Game icon fix
  • The Crossword https://www.nytimes.com/crosswords/game/daily
    • Replaced some variable colors for cells with hex colors to match the color scheme to the android app in dark mode

genius.com 
- Header: color, logo and signup link fix
nytimes.com
- Games hub: NYT Games logo, hamburger menu and progress icons fix
- Letter Boxed: game icon fix
- The Crossword: replaced some variable colors for cells with hex colors to match the color scheme to the android app in dark mode
@Sooperfish Sooperfish changed the title Update dynamic-theme-fixes.config Fixes for genius.com and nytimes.com Jan 3, 2025
@Myshor
Copy link
Collaborator

Myshor commented Jan 4, 2025

First thing: Please share URLs we can check the differences before and after fixes.
Second thing: As I found you changed some ${color} parts of fixes into static one colors then it's needed to be tested a little bit more before merged as this can (I am not saying it will for sure but it can) break the colors when light scheming in Dark Reader is used.
And last but not least: For the next time it's better to push 2 different Pull Requests for each site. Having 2 sites here in the fix will take more time to check everything + can block one part of the fix if other will need some changes.

Summarizing:
We need URLs to check changes, as screenshots for bigger changes can be not enough.

Can't seem to get [class^="puzzleProgress"]:not(puzzleProgress0) to work so I reverted to inefficient ugly but functional filtering
I'm still a little confused about the puzzleProgress fixes, reverted those for now
@Sooperfish
Copy link
Author

Sorry for the late reply. I have edited my original comment to add links, and I'll use separate pull requests next time. Thanks for the feedback.
About the usage of static colors instead of ${color}, I did this mainly because I wanted to have an accurate 1 on 1 color match to the official dark mode the android app uses. I'm not that proficient with CSS, so if there was a better way to achieve that, please let me know.
One issue: I noticed the progress icons issue on https://www.nytimes.com/crosswords persists (some of them should be inverted). I'm still a little confused what selector to use for those, so I've reverted that one for now.

Copy link
Collaborator

@Myshor Myshor left a comment

Choose a reason for hiding this comment

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

Still I found some places to fix. But it's less than last time. 😉

.vmap-layer-container > g > text
.vmap-zoom-buttons
#xwd-board
.pz-nav__logo
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is still some issue when switching from dark to light scheme and back in Dark Reader. This line should be removed.

Change this to .pz-nav__logo path

#xwd-board path {
stroke: #171717 !important;
}
#js-logo-nav > svg > rect {
Copy link
Collaborator

Choose a reason for hiding this comment

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

CSS for this selector should be removed. 😝

fill: #000 !important;
}

IGNORE INLINE STYLE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add ti this section:
#js-logo-nav > svg > rect and the logo will work better in light/dark scheme.

}
#xwd-board rect[class^="xwd__cell--cell"] {
fill: ${darkgrey} !important;
fill: #595963 !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to have this color in dark scheme then use ${#ada59b} - this will do lighter cell in light scheme.

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