Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Redoing the dark theme to be more plain CSS and not rely on central variables #8462

Merged
merged 2 commits into from
Jul 21, 2014

Conversation

dangoor
Copy link
Contributor

@dangoor dangoor commented Jul 18, 2014

This is a fix for #8420.

This turns the dark theme into a reasonably simple collection of rules that do not rely on any of the built-in LESS files.

cc: @MiguelCastillo @larz0 @peterflynn

@larz0
Copy link
Member

larz0 commented Jul 18, 2014

@dangoor @peterflynn since the inline editor is part of the core UI we shouldn't use Themes to control its colors so it shouldn't be dark since we haven't introduced the dark core UI.

It should look like this:
screen shot 2014-07-18 at 1 56 55 pm

In order to make the inline editor look like the screenshot above, we'll need to make these changes:

screen shot 2014-07-18 at 1 56 50 pm

screen shot 2014-07-18 at 1 56 08 pm

screen shot 2014-07-18 at 1 55 20 pm

screen shot 2014-07-18 at 1 54 35 pm

Hope these make sense.

@dangoor
Copy link
Contributor Author

dangoor commented Jul 21, 2014

@larz0 I had thought that inline editors were still under the control of the theme, because it is within the editor area. (So, someone writing an editor theme could choose to make inline editors look entirely different...)

@MiguelCastillo
Copy link
Contributor

@dangoor I thought the inline editor DOM was indeed inside the editor space. That should be skinnable.

@dangoor dangoor changed the title (REVIEW ONLY) Redoing the dark theme to be more plain CSS and not rely on central variables Redoing the dark theme to be more plain CSS and not rely on central variables Jul 21, 2014
@dangoor
Copy link
Contributor Author

dangoor commented Jul 21, 2014

I've updated this theme so that the inline editors are styled correctly (thanks for the pointers, @larz0!)

I think this is ready to land now. You want to review @larz0?

@larz0
Copy link
Member

larz0 commented Jul 21, 2014

@dangoor yeah I can take a look. Inline editor is a part of the core UI because it's possible for other extension to use this pattern.

If it's a part of a theme then it'd be possible for it to miss a few spots on certain extensions and the buttons within it could look out of place out of place as well because buttons are part of core UI. Hope that makes sense.

@dangoor
Copy link
Contributor Author

dangoor commented Jul 21, 2014

@larz0 Yeah, I was thinking about inline code editors, but it's definitely the case that the other inline editors need to be treated as something for UI theme and not editor theme.

It seems to me that the inline editor styles that I set in the Thor Dark stylesheet should probably live in one of the default stylesheets but only be applied when you are using a theme other than the default theme. Does that make sense to you?

@larz0
Copy link
Member

larz0 commented Jul 21, 2014

@dangoor yep that makes sense 👍

@dangoor
Copy link
Contributor Author

dangoor commented Jul 21, 2014

Actually, I don't think what we're doing works well. Maybe themes should be able to style inline code editors (but not the other inline editors). Here's the Ruby Blue theme, for example:

ruby_blue_inline

The text is not very readable. It seems like we either need to force the inline code editor to be the default theme, or we need to let editor themes style inline code editors.

@larz0
Copy link
Member

larz0 commented Jul 21, 2014

We should let the theme authors override the inline editor then.

@dangoor
Copy link
Contributor Author

dangoor commented Jul 21, 2014

FWIW, the inline color editor and bezier curve editor both look fine in Ruby Blue. The text of the inline color editor is barely readable in Thor Dark.

@MiguelCastillo
Copy link
Contributor

+1 inline editor should be themable

@MiguelCastillo
Copy link
Contributor

Maybe I am actually a bit confused... But I am not even sure how we would prevent people from theming them if inline editors are children of editor-container...

@dangoor
Copy link
Contributor Author

dangoor commented Jul 21, 2014

@MiguelCastillo it's less a matter of preventing and more a matter of what we'll document as how people should be doing things.

@MiguelCastillo
Copy link
Contributor

Ha! Ok that makes more sense :)

@MiguelCastillo
Copy link
Contributor

@dangoor the Ruby Blue theme actually reminded me of something I meant to bring up before. I would have probably done a scheme key/value in the package.json where people can specify a particular color to base the rest of the editor from... Maybe the field dark is enough, but it would be nice if the tree container matched the main editor color, for example.

@dangoor
Copy link
Contributor Author

dangoor commented Jul 21, 2014

@MiguelCastillo I remember talking about something like that earlier. I do like the idea of the editor theme suggesting colors for the UI theme to use. I think the "dark" hint is a reasonable starting point, especially because I think there would be some complexity in implementing editor theme suggested UI colors. The way production builds work right now, we can't just plug in those variables and reinterpret the UI theme because we don't interpret the LESS files at runtime.

* need a new color for some UI element, we should add a variable
* in this file.
*/
// This is the default theme and doesn't need to do anything!
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we need a newline here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha! Actually, it does require a newline. And that's the cause of #8481.

Copy link
Member

Choose a reason for hiding this comment

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

@dangoor Should we file a (post-0.42) bug on the Themes code being so finicky about trailing newlines? I could see third-party theme authors being totally befuddled by this as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. #8490 filed.

@larz0
Copy link
Member

larz0 commented Jul 21, 2014

@dangoor looks good here.

@larz0
Copy link
Member

larz0 commented Jul 21, 2014

Merging.

larz0 added a commit that referenced this pull request Jul 21, 2014
Redoing the dark theme to be more plain CSS and not rely on central variables
@larz0 larz0 merged commit 56f70a4 into master Jul 21, 2014
@larz0 larz0 deleted the dangoor/8420-theme-rejigger branch July 21, 2014 19:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants