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

bright/darkmode cookie fix #1564

Merged
merged 3 commits into from Sep 6, 2018
Merged

bright/darkmode cookie fix #1564

merged 3 commits into from Sep 6, 2018

Conversation

ovbm
Copy link
Contributor

@ovbm ovbm commented Aug 17, 2018

Fixes #1534

brightModeEnabled is enabled and application state now reads the cookie and sets state accordingly, I tried running this locally in chrome but it didn't open in darkmode after closing tab and reopening... any ideas?

@ovbm ovbm requested a review from corradio August 17, 2018 10:33
@corradio
Copy link
Member

corradio commented Aug 17, 2018

hmm so the state has brightModeEnabled = false when you open the page?

@ovbm
Copy link
Contributor Author

ovbm commented Aug 17, 2018

Ah, i misinterpreted the code.

@corradio
Copy link
Member

corradio commented Sep 4, 2018

@ovbm I've fixed the cookies. However, now, when starting in darkmode, some text is still black instead of white:

image

// Observe for search query change
observe(state => state.application.searchQuery, (searchQuery, state) => {
zoneList.filterZonesByQuery(searchQuery);
});
// Observe for brightmode change
observe(state => state.application.brightModeEnabled, (brightModeEnabled, _) => {
Copy link
Member

Choose a reason for hiding this comment

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

@ovbm This is the key change: I had to observe for a redux state change (before, we dispatched to redux but didn't listen to state changes)

@corradio corradio merged commit 20cb288 into master Sep 6, 2018
@corradio corradio deleted the ob/darkmoderemember branch September 6, 2018 14:00
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.

None yet

2 participants