Skip to content

Conversation

@Abykin
Copy link

@Abykin Abykin commented Mar 28, 2017

This fix fixes Thimble issue https://github.com/mozilla/thimble.mozilla.org/issues/1907

This line was erased in one of the previous PRs : $("body").toggleClass("dark", theme.dark);
Putting it back resolved the issue. I did some local testing to make sure nothing broke, everything seemed okay.

return result.content;
})
.then(function (cssContent) {
$("body").toggleClass("dark", theme.dark);
Copy link

Choose a reason for hiding this comment

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

this fix seems to assume a binary theme choice in that if there is any theme at all, it must be the dark theme. That will break the moment we introduce a third theme, or custom theming, so it would be better to check which theme should actually be active. Maybe @flukeout can help point in the right direction here

@flukeout
Copy link

We intentionally moved away from the using classes to set the theme, and opted for a data-theme attribute on the body instead. We may have more than two themes in the future, so the dark / not-dark binary choice doesn't make sense for the long term. Maybe we can add an override to the quickEdit CSS so that it respects the data-theme=dark attribute, and not only the class name. Will get back to you shortly on this.

Cheers!

@Pomax Pomax requested a review from flukeout March 28, 2017 18:50
@flukeout
Copy link

After thinking a bit more and chatting with @Pomax - this will be the way we handle it for now. Brackets basically has a hard-coded notion of having a 'dark theme' and the .dark class is peppered throughout the codebase. My previous attempt to move the theme to an attribute mostly worked, but didn't anticipate a lot of cases where .dark still mattered.

@flukeout flukeout merged commit aa1038e into mozilla:master Mar 28, 2017
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.

3 participants