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

Dark UI theme for inline editors #8529

Merged
merged 20 commits into from
Jul 25, 2014
Merged

Dark UI theme for inline editors #8529

merged 20 commits into from
Jul 25, 2014

Conversation

dangoor
Copy link
Contributor

@dangoor dangoor commented Jul 23, 2014

This is a spinoff of @larz0's work in #8362 with a goal of fixing #8379.

As of this initial push, I have done inline CSS and color editors. The timing editor is almost done, and quick docs remains.

cc @MiguelCastillo

@MiguelCastillo
Copy link
Contributor

@dangoor quick edit is all styled!! Nice! One thing that's more about the coloring is that it is really hard for me to see where the quick edit widget starts/ends. Maybe a bit more contrast in background color? Not sure.

screen shot 2014-07-23 at 1 16 07 pm

@marcelgerber
Copy link
Contributor

I agree with MiguelCastillo, we should add a little more contrast. We can even do it by simply adding a light ruler before and after the widget.

@peterflynn
Copy link
Member

@larz0 What do you think about the inline editor contrast? Personally I think changing the edge shadows from dark to light might look weird, but I defer to you... Any other ideas about how to improve the contrast?

One thought I had was to make the inline editor's bg slightly lighter than the main bg, rather than darker as we do in the light theme... but would that be enough to make the dark sufficiently shadows visible?

@mackenza
Copy link
Contributor

adding contrast won't really help the other themes, as they will have their own variations of dark background. If I am understanding this, the dark inline editor will be triggered by dark: true in the package.json under the theme key.

If that is right, I agree with @SAplayer 's suggestion to lighten the .top-shadow and .bottom-shadow

for top I used (255,255,255,0.2)(0,0,0,0.2)
bottom I reversed (0,0,0,0.2)(255,255,255,0.2)

image

@larz0
Copy link
Member

larz0 commented Jul 23, 2014

Give me 40 mins to try something. I'm on the streets right now.

@larz0
Copy link
Member

larz0 commented Jul 23, 2014

We should do this. (@dangoor, I think we missed a few spots because certain styles are coming from outside of the extensions in dark core UI PR).

Here's the code:

.dark .inline-widget {
color: #333;
background-color: #1b1b1b;
border-top: 1px solid rgba(255, 255, 255, 0.1);
}

screen shot 2014-07-23 at 11 31 59 am

screen shot 2014-07-23 at 11 29 00 am

screen shot 2014-07-23 at 11 30 02 am

screen shot 2014-07-23 at 11 30 22 am

@dangoor
Copy link
Contributor Author

dangoor commented Jul 23, 2014

Thanks for the feedback, everyone (and @larz0 for the styling tweak). I'll get that incorporated and finish up the remaining bits so that we can see how it feels.

@redmunds
Copy link
Contributor

@LarZ Don't forget there are 2 Timing Function Editors -- edit steps() to see the other one.

It's not a huge deal, but the bezier curve editor labels (Progression & Time) have different background colors, so it looks a bit awkward. A quick fix might be to give the text with dark bg color (Progression) a light color? Or maybe text for both labels is blue or lighter gray?

@larz0
Copy link
Member

larz0 commented Jul 23, 2014

@redmunds are you looking at #8362? Hopefully it's fixed in that PR. I'll need to check the step editor again though, think I missed that one.

@redmunds
Copy link
Contributor

@LarZ I was just looking at screenshot in this PR.

Also includes a tweak to subtly set the inline editors off from the main editor.
@marcelgerber
Copy link
Contributor

@larz0 Imho, the bezier editor shortcuts should be brightened.
And the Quick Docs contrast makes it hard to read, even w/ good eyes. But I'm not sure whether the vertical ruler should be that flashy.

@larz0
Copy link
Member

larz0 commented Jul 23, 2014

@redmunds ok cool. @SAplayer the final version should look like #8362. Ignore the screenshots for now.

@dangoor
Copy link
Contributor Author

dangoor commented Jul 23, 2014

@SAplayer I haven't done anything at all with Quick Docs yet.

@redmunds I hadn't gotten to step editor yet. I was just added the styles for that and was just about to test.

@dangoor
Copy link
Contributor Author

dangoor commented Jul 23, 2014

I just pushed changes that finish the inline timing editor and Quick Docs. There are still some tweaks needed, and I'd like to go back through #8362 and see what else might be relevant, though a bunch of the changes that remain in there are other parts of the UI that we're not worried about right now.

I took some screenshots earlier today running with #8362 for comparison purposes and what I have in my branch seems closer to usable than what I saw there. I'll post some screenshots in a moment.

@mackenza
Copy link
Contributor

I saw your changes and ran with them... I think they look pretty good. Nice work.

I am only running the default dark theme so far though... with custom themes, will the theme authors need to do anything with .inline-widget or the like?

@dangoor
Copy link
Contributor Author

dangoor commented Jul 23, 2014

dark_bezier
dark_color_editor
dark_inline_css
dark_quick_docs
dark_steps

@dangoor
Copy link
Contributor Author

dangoor commented Jul 23, 2014

@mackenza Thanks (though it's @larz0's design work, I'm just transcribing :)

At this point, theme authors should focus on their color scheme and just set the dark flag to true. If they wanted to provide an additional color scheme for the inline text editor, that would be okay. But, the other inline editors are considered "UI" (their DOMs can change any time).

@peterflynn
Copy link
Member

@dangoor The designs @larz0 posted above this morning had light borders top & bottom, while your screenshots only have light borders on top. Intentional?

@dangoor
Copy link
Contributor Author

dangoor commented Jul 23, 2014

@peterflynn No, not intentional. I'm guessing that's part of #8362 in the parts that are not in the extensions. I will be taking another look.

@mackenza
Copy link
Contributor

@dangoor I am noticing some differences in the "neatness" of the pop up editors between the default dark and my Envy and Railcasts. I guess I need to reconcile them against the new dark and see what is different. For sure the default dark is looking pretty sweet now.

@dangoor
Copy link
Contributor Author

dangoor commented Jul 23, 2014

@mackenza Yeah, it's not going to be a perfect fit with custom themes right now. We've thought that a nice partway point between full UI theming and the editor themes is that editor themes could be able to suggest some colors that could be used in place of some of the defaults. There is some infrastructure required for that first, though.

@dangoor
Copy link
Contributor Author

dangoor commented Jul 23, 2014

This is what #8362's color editor looks like on my machine:

8362

@larz0
Copy link
Member

larz0 commented Jul 23, 2014

@dangoor that's weird, thought I styled those buttons. You even picked it up in this PR.

…ark can be together when there are more themes on that list. Added proper focus style for Select element.
@larz0
Copy link
Member

larz0 commented Jul 24, 2014

I've pushed the changes I made.

@dangoor dangoor changed the title [REVIEW ONLY] Dark UI theme for inline editors Dark UI theme for inline editors Jul 25, 2014
@dangoor
Copy link
Contributor Author

dangoor commented Jul 25, 2014

I've incorporated @JeffryBooher's change which fixes inline editor code overlapping the gutter when you scroll horizontally (#8541).

@dangoor
Copy link
Contributor Author

dangoor commented Jul 25, 2014

I'm not sure if this needs additional review at this point? This pull request has gone through a lot of testing, @larz0 has seen what I've done and I've seen what he has done. I think this is ready to go.

@mackenza
Copy link
Contributor

I am looking to make sure my custom themes are reflective of these changes and in the new main.less for the darkTheme I see:

/* Inline editor styling */

.related-container .selection:before {
    border-top: 9px solid transparent;
    border-bottom: 9px solid transparent;
}

.inline-widget .CodeMirror, .inline-widget .CodeMirror-gutters {
    background: transparent;
}

are those going to remain in the actual theme stylesheet or move to the default styles? They don't seem to have anything theme specific in them so that's why I was wondering if I needed them in my theme stylesheet.

@larz0
Copy link
Member

larz0 commented Jul 25, 2014

@mackenza you don't need to add them to your stylesheet unless you want to override them on purpose.

@marcelgerber
Copy link
Contributor

@larz0 Some spots I found:

  • Quick View popver
    image
  • steps editor (and bezier editor) .info .hint color
    image
  • image custom viewer
    image

@dangoor There are still some issues left open, like the dark scrollbars in Dialogs.

@larz0
Copy link
Member

larz0 commented Jul 25, 2014

@SAplayer Quick View and image viewer won't make it in 42 but I'll fix steps editor hint color. Nice catch!

@larz0
Copy link
Member

larz0 commented Jul 25, 2014

@SAplayer image viewer styling will be in release 42 👍

@redmunds
Copy link
Contributor

Copyright date for LightTheme/main.less should be changed to 2014

@larz0
Copy link
Member

larz0 commented Jul 25, 2014

@redmunds fixed.

@redmunds
Copy link
Contributor

I made a pass through the code. Looks good.

@dangoor
Copy link
Contributor Author

dangoor commented Jul 25, 2014

I just pushed a change to constrain the dark scrollbars to #editor-holder. I tried testing by setting the class on body to .platform-win, but the editor wasn't showing custom scrollbars for me, though the styles seemed to have been applied in the dev tools. This change did fix the scrollbars in the dialogs.

Can someone test this? I'm going to be offline for a while now. As far as I can see, once the scrollbars look good, we can merge.

@marcelgerber
Copy link
Contributor

@dangoor The scrollbars are working fine and are only applied to the editor.

@redmunds
Copy link
Contributor

That fixes all of the scrollbar cases I was seeing. Merging.

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.

10 participants