Skip to content
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

fix dark mode colour shorthands #269

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

crjc
Copy link
Contributor

@crjc crjc commented Feb 6, 2024

While in dark mode, opacity shorthands would not work correctly.

For example: bg-white dark:bg-white/50

dark:bg-white/50 would get kind = ordered
bg-white would get kind = dependent and override the above

The fix I made works for me, but not sure if it's the 'right' fix.

@jaredh159
Copy link
Owner

thanks for the PR.

took me a little while to wrap my head around this one, to make sure i understood why the fix was needed, and if your approach was the correct one. after debugging a little bit, i think your approach is good enough, without making deep changes.

the issue is that the test case you found that was broken mixed a utility with no opacity (bg-white) with a dark-mode utility that also included an opacity shorthand (dark:bg-white/50). the former gets categorized as a dependent styleIr because we have to wait and see if we need to merge in a utility like bg-opacity-50. but the latter we know is complete, because the shorthand was supplied.

but, this causes an issue, because we currently resolve dependent IRs after all of the ordered ones. the dark mode utility gets wrapped as an ordered IR because it needs to be resolved after any non-dark mode utils. but, we currently process all dependent IRs after the ordered ones, so we have a sort of intractible situation, which seems like a flaw in the current resolution algorithm.

all that said, converting the shorthand to also be dependent fixes things by pushing it back to the end of the dependents queue to be resolved last. so, while it's a bit wierd, i can't think of a simpler, cleaner fix. so i'm going to merge this, and i then i'll probably add a bit of internal documentation for my future self.

@jaredh159 jaredh159 merged commit 02cb50d into jaredh159:master Feb 12, 2024
1 check passed
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