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

Feat/interpreted style for color schemes #534

Merged

Conversation

tarnung
Copy link
Collaborator

@tarnung tarnung commented Oct 25, 2020

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.

@munen
Copy link
Collaborator

munen commented Oct 28, 2020

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:

image

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:

image

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.

@munen
Copy link
Collaborator

munen commented Nov 6, 2020

Hm.. I've merged master which has the fix for using defaults. But the snapshot still fails.

@tarnung
Copy link
Collaborator Author

tarnung commented Nov 6, 2020

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.
So it seems that colors.css is not loaded and the theme setup function is not run either.

@munen
Copy link
Collaborator

munen commented Nov 7, 2020

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.

@tarnung
Copy link
Collaborator Author

tarnung commented Nov 7, 2020

Thank you for the effort.
One other possibility to specifically fix the test would be to write the background-color to redux and read it from there. Then there would be no need to read it from dom too.
I agree that setting the css variables is probably the least offensive use of direct dom manipulation in organice. So I think it's ok to update the snapshot.

@munen
Copy link
Collaborator

munen commented Nov 8, 2020

One other possibility to specifically fix the test would be to write the background-color to redux and read it from there.

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.".

I agree that setting the css variables is probably the least offensive use of direct dom manipulation in organice. So I think it's ok to update the snapshot.

We're on the same page here. Let's merge your nice changes before they get stale(;

@munen munen merged commit 3e3f292 into 200ok-ch:master Nov 8, 2020
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