Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style code blocks #210

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

HugoGranstrom
Copy link
Collaborator

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?

@HugoGranstrom
Copy link
Collaborator Author

HugoGranstrom commented Jul 11, 2023

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
Copy link
Owner

@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!

@@ -20,6 +20,8 @@ proc useHtmlBackend*(doc: var NbDoc) =
doc.partials["nbCapture"] = """{{>nbCodeOutput}}"""
doc.partials["nbCodeSource"] = "<pre><code class=\"nohighlight hljs nim\">{{&codeHighlighted}}</code></pre>"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this line and the following be removed?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(from here it is not clear but later lines seem to override this)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct 👍 Will delete them

@HugoGranstrom
Copy link
Collaborator Author

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 🤔 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.

@HugoGranstrom
Copy link
Collaborator Author

The code ain't pretty, but the code blocks are 🙃 I'll fix the tests now.

@HugoGranstrom
Copy link
Collaborator Author

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?

@HugoGranstrom
Copy link
Collaborator Author

I fixed another thing that has been a bit ugly: overflowing outputs! Penguins looks really slick now: https://c8ca2d62fa794955ee68--nimib.netlify.app/penguins

@pietroppeter
Copy link
Owner

pietroppeter commented Aug 17, 2023

I was looking on how to improve borders of dark theme and I am able to get to this:
image
instead of this:
image
in order to do that I had to:

  • change border-color to var(--border) in .nb-code-pre and .nb-output
  • remove the background change from android studio css (see screenshot below), I think we should be able to do that adding a rule to our style that says that for class .hljs the value of background should be var(--background).
    image
    (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:
image
(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!

@pietroppeter
Copy link
Owner

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.

@HugoGranstrom
Copy link
Collaborator Author

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 😄 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:
image
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.

@HugoGranstrom
Copy link
Collaborator Author

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... :/

@pietroppeter
Copy link
Owner

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

@HugoGranstrom
Copy link
Collaborator Author

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 🤔

@HugoGranstrom
Copy link
Collaborator Author

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.

@pietroppeter
Copy link
Owner

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 🤔

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.

@HugoGranstrom
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants