search

JunoLab / atom-ink

IDE toolkit for Atom
MIT License
228 stars 40 forks source link

update the spinner coloring #11

Closed pfitzseb closed 9 years ago

pfitzseb commented 9 years ago

so it works on all themes.

MikeInnes commented 9 years ago

Looks like a good change. When I had something like this it came out a little grey, though, so maybe text-color-highlight from ui-variables would look better?

pfitzseb commented 9 years ago

Nah, text-color-highlight is something from ui-variables and won't play nicely with syntax themes (which define the colors in syntax-variables). So when you have a e.g. One Light as UI Theme and One Dark as Syntax Theme you'd get a black spinner on a grey background. Using something from sytax-variables seems like the Right Thing™ to me.

MikeInnes commented 9 years ago

True, but ink already essentially makes the assumption that your ui and syntax themes are consistent – for inline results, for example – so I think we can feasibly just recommend that people stick to that. I don't really think there's a way around that when you're sticking UI in the editor.

Perhaps another option would be to detect whether the syntax background is light and use white or black depending on that. Less probably supports some kind of if / condition for that sort of thing.

pfitzseb commented 9 years ago

Huh, but why? It isn't all that much trouble changing the inline results to something that's defined by the syntax theme (in fact, I think I already have done that locally).

MikeInnes commented 9 years ago

Basically because inline results are UI, and not syntax. It's not a big deal now, but if inline results start to contain more UI elements – say progress meters, or buttons and sliders via Interact.jl – it starts to get harder to build that off the syntax theme in a nice and non-hacky why.

Anyway, separate issue. This is still a much-needed improvement so we can go ahead with it.

pfitzseb commented 9 years ago

I'm not completly convinced that it'd be all that hard to build a UI with like three colors (background, foreground, highlight) -- especially since we can do quite a lot of transformations with less preprocessing--, but we'll have to see about that ;)

But I agree, that's an issue for another time. Merging.

MikeInnes commented 9 years ago

Yeah, we definitely have to play around with that stuff a bit more at some point.

Thanks for the patch!