-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
Feat/interpreted style for color schemes #534
Feat/interpreted style for color schemes #534
Conversation
Thank you for this PR, @tarnung! It's a good change and I want to merge it, but this PR shows one issue we have introduced before: At least since switching from 'darkMode' yes/no to 'colorScheme' 'light'/'dark', we had no default colorScheme. This is the reason for the tests to fail, too: Since there is no default colorScheme, the function you wrote defaults to black (which is reasonable!). I propose that we should fix the issue of not having a default colorScheme in this PR or in parallel. Otherwise, we'll have to 'break' the test in order to have green tests. This is what it looks like for the user at the moment if she has not selected one, yet: The 'solarized light' theme is visibly active, but no color scheme is selected in the settings. Likely, this will get more confusing as soon as there's more color schemes being introduced. I have added a 'partial fix' in 57061cb. It's only partial, because it's the place where this information should be. Unfortunately it is not applied. I would need additional time for investigating this. |
Hm.. I've merged |
I don't know exactly what is going on, but i debugged it so far that I can say that during testing reading the css variable returns an empty string. |
Ok, I looked into it some more. So, we have a architectural issue. Generally, in organice we're using React to create Views and don't use HTML5 APIs that mutate the DOM. For the color theme implementation, we do: const style = document.documentElement.style;
Object.entries(themes[theme][colorScheme]).forEach(([k, v]) => style.setProperty(k, v)); That mutates the DOM in place. We cannot do that in a test. Just calling the code from the test runner will not have the side-effect that would happen in a browser. I've spent some time thinking and came to the conclusion that the required changes would be pretty big, because we couldn't use that API. Instead we would have to build a styled component that wraps the current application. Since the current implementation does work well and this is the only test that's failing, I have updated the snapshot in place and added documentation around the previously failing test. |
Thank you for the effort. |
True. Doing this for all colors in all cases would lead to the option "Instead we would have to build a styled component that wraps the current application.".
We're on the same page here. Let's merge your nice changes before they get stale(; |
Style interpolation for the swipe action assumed light mode background color as a starting point. I added a function that reads out the defined background color.
It's not the most elegant fix. Maybe there is a pure css solution to the whole style interpolation but I have no expertise in this area.