jannis-baum / Vivify

Bring your Markdown to life
GNU General Public License v3.0
34 stars 4 forks source link

GitHub alert block quotes #118

Closed tuurep closed 3 months ago

tuurep commented 3 months ago

Close #89

Tweekism commented 3 months ago

Lookin goooood, I'm excited 😋

tuurep commented 3 months ago

Ok @Tweekism @jannis-baum this is ready for feedback

I basically took what's here:

https://github.com/antfu/markdown-it-github-alerts/blob/main/styles/github-base.css

But fit it into our style.css, maybe removing some redundant lines

Screenshots:

ghalert1 ghalert2

My concerns:

jannis-baum commented 3 months ago

Hey thanks! I will look into the code later or tomorrow :)

Would there be a way to plug the styles from the plugin without essentially copy pasting CSS? Are they on the repo just for reference or supposed to be able to be used directly?

Not sure. On giving their docs one short look I couldn't find instructions. Might also be that our set up is a bit too exotic for this to work. One cheeky thing you could try however is making a symlink in the static directory to their styles at node_modules/....css and then adding that symlink to be included in the as styles in our viewer.ts ^^ Might just work. If it works on dev but not for the build we can fix it. If it doesn't work on dev it would get a bit ugly I think

Tweekism commented 3 months ago
  • What do you think about those ANSI colors on dark? To me looks a bit hit and miss.. Some are ok, but some too light, notably yellow.

    • They are kinda ok though :)

These colors are workable, I missed the context for why ANSI colors. Is the intention to use the same colors in the terminal in vim?

jannis-baum commented 3 months ago

These colors are workable, I missed the context for why ANSI colors. Is the intention to use the same colors in the terminal in vim?

From https://github.com/jannis-baum/Vivify/issues/89#issuecomment-2230750793 onwards. We were thinking to reuse ANSI-colors because we also need them for rendering Jupyter Notebooks.

What do you think about those ANSI colors on dark? To me looks a bit hit and miss.. Some are ok, but some too light, notably yellow.

I honestly don't like them that much to be honest🙈

tuurep commented 3 months ago

What do you think about those ANSI colors on dark? To me looks a bit hit and miss.. Some are ok, but some too light, notably yellow.

I honestly don't like them that much to be honest🙈

That's a good answer :smile: What kind would work best?

My thought is that they could be like the light theme colors but adapted to dark, then adjust and whatever

Wdyt about that or did you have something else in mind?

I can make an initial suggestion if needed

jannis-baum commented 3 months ago

That's a good answer 😄 What kind would work best?

My thought is that they could be like the light theme colors but adapted to dark, then adjust and whatever

Wdyt about that or did you have something else in mind?

I can make an initial suggestion if needed

I honestly don't have any idea ^^ Would be glad about a suggestion! I did see however that the plugin also has a dark theme CSS (https://github.com/antfu/markdown-it-github-alerts/tree/main/styles) so if you want to go the route of adjusting the light colors to dark that might already be done there :)

tuurep commented 3 months ago

One cheeky thing you could try however is making a symlink in the static directory to their styles at node_modules/....css and then adding that symlink to be included in the as styles in our viewer.ts ^^ Might just work.

Seems like such hackery :smile: Maybe it's worth it to have all the CSS in there with the rest and avoid this?

But I'm kinda interested in what the function of the CSS in the repo is. Whether that's some markdown-it plugin convention and there's more info about that. I'll see.

tuurep commented 3 months ago

I did see however that the plugin also has a dark theme CSS (https://github.com/antfu/markdown-it-github-alerts/tree/main/styles) so if you want to go the route of adjusting the light colors to dark that might already be done there :)

Yeah I can try this as an initial step

The dark theme is "GitHub Dark" and notable that it has quite a different background color than yours

I could also look at GitHub Dark High Contrast alert colors

jannis-baum commented 3 months ago

But I'm kinda interested in what the function of the CSS in the repo is. Whether that's some markdown-it plugin convention and there's more info about that. I'll see.

I think it's some bigger thing than just markdown-it; I have definitely seen similar ways of importing CSS in NextJS and React so there is probably some big widely used library that does this. I don't know how applicable it is to our use-case of serving the file statically with express though. If we want to do that we might have to change quite a bit of stuff and start using JSX with some other dependency. But yes, maybe it's less bad; might be worth investigating!

jannis-baum commented 3 months ago

The dark theme is "GitHub Dark" and notable that it has quite a different background color than yours

Good point!

I could also look at GitHub Dark High Contrast alert colors

Yes true. Might just be that those colors "pop" a bit too much but maybe it's a good starting point anyways.

tuurep commented 3 months ago

:warning: I'll go deep into GitHub colors

The markdown-it plugin repo CSS has slight differences in colors to what they actually are on GitHub as of this moment, but it does look like they were taken from the GitHub variables directly, ~8 months ago:

https://github.com/antfu/markdown-it-github-alerts/commit/595f57831879f6058bf5df430357747b21cdfd74

(due to the same variable names that you see when you inspect GH)

Speculation: I guess that GH adjusts these colors over time, and CSS such like in the plugin doesn't keep up

So I would take the colors straight from inspecting GH. But lets look at screenshots for:

  1. Plugin CSS
  2. Straight from GitHub CSS
    • For dark: GH Dark and GH Dark High Contrast

Light

Plugin repo CSS

image

GitHub up-to-date CSS

image

Dark

Plugin repo CSS

image

GitHub up-to-date Dark CSS

image

GitHub up-to-date Dark High Contrast CSS

image

GitHub up-to-date Dark Dimmed CSS (basically Low Contrast)

image

tuurep commented 3 months ago

So in my opinion any of the last 3 (note: I added a "low contrast") could work for dark, and all look pretty good

I would avoid the outdated plugin CSS

Thoughts @jannis-baum ?

tuurep commented 3 months ago

Default:

#1f6feb
#238636
#8957e5
#9e6a03
#da3633

High contrast:

#409eff
#09b43a
#b87fff
#e09b13
#ff6a69

Dimmed:

#316dca
#347d39
#8256d0
#966600
#c93c37

Tweekism commented 3 months ago

Ok yeah the up to date dimmed does look a lot nicer.

tuurep commented 3 months ago

Yeah it could be a fit!

To be perfectly clear the Dimmed one is not something I created, but it's a theme on GitHub:

image

jannis-baum commented 3 months ago

Hey, sorry I've been taking so long to reply to this, I've been back and forth on the colors because I am not that happy with any of them. They all seem a bit dull to me and the warning color is very brown.

I just checked through Jellyfish again and realized I have colors for compiler notes, warnings and errors that might be quite suitable. So if we use those three plus the ANSI colors for "tip" and "important" it would look like this:

image
:root {
    --color-alert-note: #afafff;
    --color-alert-tip: #b4fa72;
    --color-alert-important: #ff8ffd;
    --color-alert-warning: #ffaf00;
    --color-alert-caution: #ff5f5f;
}

What do you think?

Tweekism commented 3 months ago

I'm a little triggered that Note has shifted from blue to more of a purple 😂

And it might be a little too similar to Important?

tuurep commented 3 months ago

I was just about to comment too

It's definitely an improvement from the first

(this) alert1

I think it's pretty fine tbh and I'd approve, but if we need to adjust then lets think about it

Safe to say lets not use the GH colors directly, if they look out of place in the theme

jannis-baum commented 3 months ago

Soooo how about falling back to the ANSI color for Note as well?

image

:root {
    --color-alert-note: #a5d5fe;
    --color-alert-tip: #b4fa72;
    --color-alert-important: #ff8ffd;
    --color-alert-warning: #ffaf00;
    --color-alert-caution: #ff5f5f;
}
Tweekism commented 3 months ago

I like it, I like that our pop a little more. I'd be happy with either honestly.

I know no one was asking, but here is an up close comparison with the git hub ones.

image

Tweekism commented 3 months ago

Its easy to tweak a single colour in custom css, but it is important to have sensible defaults

tuurep commented 3 months ago

it is important to have sensible defaults

Yeah I agree with this very much :)

I have a little feeling that the blue is just a bit too light, similarly to the ANSI yellow but not nearly as severe.

But the more I've stared it and tried to come up with an improvement, the more I think it's kinda OK enough.

jannis-baum commented 3 months ago

Hahaha okay so let's just merge this? In case we end up disliking it too much we can tweak it when we refactor the CSS in #120. That one will probably be a huge color discussion anyways😂

jannis-baum commented 3 months ago

Oh but before we merge, @tuurep do you want to commit the latest version we agreed on now? I.e. ANSI plus the warning and error colors.

:root {
    --color-alert-note: #a5d5fe;
    --color-alert-tip: #b4fa72;
    --color-alert-important: #ff8ffd;
    --color-alert-warning: #ffaf00;
    --color-alert-caution: #ff5f5f;
}
tuurep commented 3 months ago

Alright, done!