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 #1907: menus don't have a dark theme anymore #670

Merged
merged 1 commit into from Mar 28, 2017

Conversation

Projects
None yet
3 participants
@Abykin

Abykin commented Mar 28, 2017

This fix fixes Thimble issue mozilla/thimble.mozilla.org#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.

@@ -237,6 +237,7 @@ define(function (require, exports, module) {
return result.content;
})
.then(function (cssContent) {
$("body").toggleClass("dark", theme.dark);

This comment has been minimized.

@Pomax

Pomax Mar 28, 2017

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

@Pomax

Pomax Mar 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@flukeout

flukeout Mar 28, 2017

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!

flukeout commented Mar 28, 2017

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 Mar 28, 2017

@flukeout

This comment has been minimized.

Show comment
Hide comment
@flukeout

flukeout Mar 28, 2017

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 commented Mar 28, 2017

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment