Open HugoGranstrom opened 1 year ago
To clarify the problem look at this example were I have hard-coded the background color:
And here are the code blocks using the default (I think it comes from highlight.js) background color:
Looking at these, I think it's a no-brainer to go with the default code background. The difference is not big for the light theme, and it looks reasonable for the dark theme. The border color is sub-optimal on the dark-theme though :/ As I said above, we could set different colors (keep this one for light theme and select a darker one for dark theme) for our two themes, but this wouldn't play well with for example NimiTheme.
@pietroppeter What is your opinion on this?
this looks cool!
Looking at these, I think it's a no-brainer to go with the default code background.
agree
The border color is sub-optimal on the dark-theme though :/
also agree
we could set different colors (keep this one for light theme and select a darker one for dark theme) for our two themes, but this wouldn't play well with for example NimiTheme
yeah, it would be hard to check every stuff in nimitheme how it is impacted. should we keep an option to revert to old style, maybe (although I am starting to dislike complexity of all these options, at some point we will need to pay some debt and simplify stuff)? I feel a better solution might like in css variables or something like that but I do not know enough about it (and we do not need to work on this now).
As further feedback if possible I would make sure that a nb-code not followed by a nb-output should have rounded corners (I guess it should not be too hard with an override using some css selector...).
and thanks for working on this, it definitely looks like an improvement!
yeah, it would be hard to check every stuff in nimitheme how it is impacted. should we keep an option to revert to old style, maybe (although I am starting to dislike complexity of all these options, at some point we will need to pay some debt and simplify stuff)? I feel a better solution might like in css variables or something like that but I do not know enough about it (and we do not need to work on this now).
Yes, it's a hard nut to crack :/ A plugin system could solve part of the problem as you could choose some styles but not others to use in your theme. For example, if I want to default nimib style of images but wanted my custom style of nbCode. CSS-variables could actually work though :thinking: They would need to be defined before the styles though, so it would require us adding a dedicated {{cssVariables}}
section above {{nbStyle}}
. Then we could define the background color of code blocks as a variable with a default value which can be edited by the user.
As further feedback if possible I would make sure that a nb-code not followed by a nb-output should have rounded corners (I guess it should not be too hard with an override using some css selector...).
Yes it should be solvable in mustache using different classes, but I'm not sure about how it could be done using only CSS.
The code ain't pretty, but the code blocks are :upside_down_face: I'll fix the tests now.
Previously, we haven't really styled the code and output as a single unit, but now that we do, I think it would make sense to put them inside a <div>
to encapsulate them. Right now I'm getting a weird formatting for a nimibCode
without output followed by a nbCapture
(which uses {>nbCodeOutput}}
) which causes it to style incorrectly: https://b7ba08db5fd951a4111a--nimib.netlify.app/allblocks
What I suggest is basically that we define:
doc.partials["nbCode"] = """
<div class="nb-code-wrapper">
{{>nbCodeSource}}
{{>nbCodeOutput}}
</div>"""
This could potentially break custom styles but I don't think the risk is that high. What do you say?
I fixed another thing that has been a bit ugly: overflowing outputs! Penguins looks really slick now: https://c8ca2d62fa794955ee68--nimib.netlify.app/penguins
I was looking on how to improve borders of dark theme and I am able to get to this: instead of this: in order to do that I had to:
border-color
to var(--border)
in .nb-code-pre
and .nb-output
.hljs
the value of background
should be var(--background)
.
(this last change will probably affect also light background but I think it will be for the better, it means we keep using the background value defined by the overall theme - water.css or another coming from nimitheme - instead of that of the hljs theme; in particular for dark background the contrast I think it was not good enough)What do you think of the above?
Another thing that now looks a bit odd is the difference between code generated from nbCode
blocks and similar and simple html pre code
blocks, which do not have a border:
(like it happens with nbFile but see also cheatsheet).
I do not think this last one is a big issue but we could note it down to solve it later unless you have a fix already.
Other than those two remarks, overall looks like a very nice improvement!
This could potentially break custom styles but I don't think the risk is that high. What do you say?
In the end you did not need this nb-code-wrapper
right? at some point I think we will have some kind of div wrapping content of a block (I would go with something generic for all blocks), but I would wait for when we have an actual use case for that before designing how it would look.
I was looking on how to improve borders of dark theme and I am able to get to this:
That looks much better! Thanks for looking in to this :smile: I'll try and implement this today!
(this last change will probably affect also light background but I think it will be for the better, it means we keep using the background value defined by the overall theme - water.css or another coming from nimitheme - instead of that of the hljs theme; in particular for dark background the contrast I think it was not good enough)
Yes, if it looks decently good at least, I'm all for using variables. We just have to document somewhere that themes must define --border
and --background
.
Another thing that now looks a bit odd is the difference between code generated from nbCode blocks and similar and simple html pre code blocks, which do not have a border:
Yeah, I've noticed :/ I could give it a shot trying to do it without classes, but I don't think you can select a parent using CSS currently (i.e. I can't style the pre
based on it having a code
child, only the other way around works pre > code
). There is :has(), but it's not enabled by default in Firefox (my browser of choice B-) ) yet.
I do not think this last one is a big issue but we could note it down to solve it later unless you have a fix already.
So yes, I think it's hard to solve at the moment :/
In the end you did not need this nb-code-wrapper right?
No, we can still get things like this:
But I really think the solution in this case specifically would be to use a different partial than reusing nbCodeOutput
and instead create a dedicated textbox with the same border style.
it means we keep using the background value defined by the overall theme - water.css or another coming from nimitheme
We have a problem, I picked a few random nimitheme themes and none of them defined neither --border
nor --background
:/ So we/neroist would have to manually assign these for all themes... :/
Well we can use the current values as fallback values: https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties#custom_property_fallback_values
We should indeed document which variables we use for our custom nb styling
Well we can use the current values as fallback values:
Ohh, good find! But we still have problems. We can only set one fallback value. So we are back to the problem of light and dark themes. The dark themes would look horrible if we chose a light background as fallback :thinking:
And I have checked, there seems to be no way of doing "if this variable doesn't exist, use the previously defined value". It can only be a specific value, or the default value.
Well we can use the current values as fallback values:
Ohh, good find! But we still have problems. We can only set one fallback value. So we are back to the problem of light and dark themes. The dark themes would look horrible if we chose a light background as fallback :thinking:
Well our default is light and dark is set by useDark, maybe something can be done taking advantage of that?
Anyway I think this is not too bad if something needs to be fixed either in nimitheme or customly, we can only do so much.
Well our default is light and dark is set by useDark, maybe something can be done taking advantage of that?
Not sure tbh, it's only the default water.css themes that use nb.darkMode
(I assume that's what you meant because I didn't find useDark
). And those are exactly the themes that we know have these CSS variables defined already.
Anyway I think this is not too bad if something needs to be fixed either in nimitheme or customly, we can only do so much.
As long as that "something" isn't too cumbersome for @neroist to change, I agree. One big problem is also that nimitheme has the feature of choosing the highlight.js theme, which we will screw over here with our override of the code background which they can't do anything about.
Maybe the solution is to not change the background manually, but instead choose a better default highlight.js theme for water.css dark mode? Then we only have to care about the border-color, which is more forgiving.
This is my attempt at implementing code blocks that look something like the DeepNote blocks in #65 . The styling still needs some tuning (feedback is welcome) and there are some things we might have to consider. For example the colors (the background color of the code and the border color), if I hardcode them as I do now, they will look bad in dark mode for example. Add to that all themes in NimiTheme. If we hard-code them, it may look weird with any theme but the default light water.css theme. So we might have to go with using the default background color of the current theme. The border color could probably work if we set it to a grey-ish color, otherwise we get a fully black/white border which is not a good look.
@pietroppeter What is your opinion on this?