Oni: Add styling option for "dirty" marker in tabs

[hoschi]

After @CrossR fixed #977 it would be great to give that another color and I can't find one in https://github.com/onivim/oni/blob/master/extensions/theme-onedark/colors/onedark.json

With solarized theme it is hard to see:

[divayprakash]

@hoschi I'd like to take this up. Could you give me some more information on how to proceed?

[akinsho]

@divayprakash that'd be awesome if you wanted to take this on we'd really appreciate it. Here is the relevant link to the actual react component I think (95% certain), at the moment its a normal react component but I think re how to implement this steps would be

1. In Tabs.tsx I would create a styled-component as we do here and using the theme as in the link, experiment with one of the available theme options maybe the editor.highlight.mode.background.
2. For Bonus points: If you're interested, creating a user specifiable option, if you're interested in trying this let me know as that would be a bit more involved

[singhpratik17]

Hey, I guess this hasn't been resolved yet. If it isn't, I would like to take this up as my first contribution. Does that work?

[akinsho]

@singhpratik17 that would be amazing 👍 lemme know if you need anymore details about where to go with this

[wheyerstrass]

I'm playing around with this.
I made a style-component like this:

const DirtyMarker = styled.div`
  background-color: ${props => props.theme["foreground"]} !important;
`

I then replaced the current dirty marker, which is:

<div class="circle" />

( circle sets background-color: #c8c8c8; )
with

<DirtyMarker className="circle" />

Here are some examples:

@hoschi Look at the available colors and give me your feedback
https://github.com/onivim/oni/blob/master/extensions/theme-solarized/colors/solarized8_light.json

Having the user select the color could be done by extending the config reference e.g.
"ui.editor.noidea.dirtymarker.color": "#2341512" (or use the color-keys directly), but I find this weird as it might work for one specific colorscheme but not for others.
This seems like pure color fix due to cirlce class being fixed.

@Akin909 I'd appreciate your feedback.

[akinsho]

@wheyerstrass very cool thanks for having a look at that, re. the circle class if it isn't being used anywhere else I would just take the styles out of that and add it to your styled component we are slowly trying to move over all our less to styled components so that will help.

So rather than doing !important you could just set the color to whatever you choose and do away with the extra circle classes color (I personally like the green but it might have to come out of the theme so maybe set it to the normal highlight color then unless the user specifies a theme color it defaults to that.

For this you'd have to and add the dirty marker color option to the Default configuration.ts,
and ConfigurationValues.ts which is where the types are defined so in the component it would look like

    const DirtyMarker = styled<{ userColor?: string}, "div">("div")`
        //circle styles
        background-color: ${p => p.userColor || p.theme.normal.foreground(or whaterver)};
    `
// NB: the userColor would be a prop you passed down e.g.
const userColor = configuration.getValue("oni.tabs.dirtyMarker.color")
<DirtyMarker userColor={userColor} />

[hoschi]

@wheyerstrass nice job! I like the greenish version, should be #2aa198 :grinning:

[wheyerstrass]

@hoschi Semantically, that would be the color of the insert mode - don't you think it would be confusing ? Using the themes foreground color seems more appropriate.

I'd also question, wether to give the user the option to choose a color at all - isn't this what the theme is for ?
Am I being too pedantic about this small change ?

[akinsho]

@wheyerstrass at the end of the day its up to you we can always see what users end up wanting but some color as an initial goal is 👍, I think the ability to choose would be cool but requires a bit more legwork and adds more code to maintain (minimal) but still so either way has tradeoffs, re color having had issues with #2157 I think it should definitely come out of the theme but the foreground color might cause it to blend in too much and be hard to see on some themes

[hoschi]

@wheyerstrass @Akin909 good points both of you. I find it important to see modification color, because of that I would like the green more than the black (foreground) color. Which answers the "should it be changeable" question :grin: It should be as different people focus/like different things/styles. My suggestions:

• make it changeable
• use foreground color as default, because for most themes this should be outstanding enough

Deal?

[wheyerstrass]

Jep, I'm cool with either - just getting the hang of everything.

[wheyerstrass]

I created a pull request: #2391
Tell me what to improve ( I'm currently thinking about tests ).

[akinsho]

I think this can now be closed do you agree @hoschi?

[hoschi]

Yes

Akin [e-mail] schrieb am Mo., 16. Juli 2018, 11:46:

I think this can now be closed do you agree @hoschi
https://github.com/hoschi?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/onivim/oni/issues/1580#issuecomment-405196806, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAJ9ONDLkxLeF_jdenA6MpfANqiFWU4lks5uHGEEgaJpZM4SJwE1
.

[akinsho]

closed by #2397