Update water.css and fix code output #185

dlesnoff · 2023-03-05T17:40:48Z

Here is a PR with the changes proposed by @HugoGranstrom in #184 .
Fix #184 .

HugoGranstrom · 2023-03-05T18:23:39Z

Nice! The only thing would be that we should create a specific class if we want to use samp for something else in the future. Or what do you think @pietroppeter?

pietroppeter · 2023-03-06T06:19:08Z

I do not see what has changed I suspect it might be different on different OSes, can we add screenshots of before and after? In particular on my side I still see the back tick content with a bigger font than the rest of the text as before (which is not what I see in water.css page). What was the change that samp as display was supposed to fix (for the moment I would not bother to make output a class, if we need we can do it later)? Should we also try removing normalize.css?

HugoGranstrom · 2023-03-06T06:23:50Z

Oh, the bigger font size is because of normalize.css still being used. The samp previously just looked like a inlined backticked string spanning multiple lines (ugly). Now it takes up the full width of the page.

pietroppeter · 2023-03-06T07:52:33Z

Ah I see about the samp. Would it make sense instead to wrap it in a pre block?

pietroppeter · 2023-03-06T07:54:55Z

Mmh actually I see it is already wrapped in a pre block with class nb-output. I guess that did not solve it? I actually never noticed this issue about the samp.

pietroppeter · 2023-03-06T08:06:44Z

mmh, actually reading about samp I see it is supposed to be an inline element. I guess we could skip the samp and wrap only in pre?

dlesnoff · 2023-03-06T13:41:17Z

On Linux, if I change only the water.css, I get an horrible code output. I am no CSS expert, but I guess we can wrap output both by samp and pre ? Is the display: block trick not functioning on your end? Le lun. 6 mars 2023 à 09:06, Pietro Peterlongo ***@***.***> a écrit :

…

mmh, actually reading about samp <https://developer.mozilla.org/en-US/docs/Web/HTML/Element/samp?retiredLocale=it> I see it is supposed to be an inline element. I guess we could skip the samp and wrap only in pre? — Reply to this email directly, view it on GitHub <#185 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANDHQOCL37YT63EVIJXV3K3W2WLJ7ANCNFSM6AAAAAAVQJJBWY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

HugoGranstrom · 2023-03-06T19:24:23Z

I actually never noticed this issue about the samp.

It's because it hasn't been a problem until now that water.css has added a background to the samp. We just couldn't see that it wasn't a block before.

mmh, actually reading about samp I see it is supposed to be an inline element. I guess we could skip the samp and wrap only in pre?

If we want to have a background color (that is what water.css added), we will have to put the background on the pre tag and not the samp tag. So we have basically two options:

Keep samp but override water.css's styles to the default values. Add background to .nb-output class.
Remove samp and just use pre. Add background to .nb-output class.

If it works without the samp I think we should go with option 2.

I am no CSS expert, but I guess we can wrap output both by samp and pre ?

That is what we are currently doing. But as water.css only styles the samp, it looks ugly because it is an inline element. Changing it to display: block works, but at this point we might just as well do this properly and remove the samp.

This is the water.css's CSS for samp:

samp {
  background: #efefef;
  background: var(--background);
  color: #000;
  color: var(--code);
  padding: 2.5px 5px;
  border-radius: 6px;
  font-size: 1em;
}

I would say that we take these attributes and inserts them into our .nb-output class defined in themes.nim in the variable nbStyle.

What do you both think?

pietroppeter · 2023-03-07T20:03:50Z

Yep option 2 has my vote too (if it works). Thanks for the explanation Hugo. To be honest not sure if we want to add a background or we keep it white. Screenshots with options would help pick.

HugoGranstrom · 2023-03-08T18:10:14Z

With dark background:

With same background:

With lighter background:

With no background:

HugoGranstrom · 2023-03-08T18:28:03Z

And a conceptual one with borders around the code+output block and labels:

HugoGranstrom · 2023-03-08T18:28:53Z

Compared to the others, I actually think the current style without any background looks pretty bad 😅

pietroppeter · 2023-03-08T19:09:37Z

I don't know. What I am not too fond about the versions with background is that output and code seem two separate elements.

An old issue which is relevant is #65. There I linked a deep note example where the link between output and code is made visible through a light border.

HugoGranstrom · 2023-03-08T19:26:56Z

I don't know. What I am not too fond about the versions with background is that output and code seem two separate elements.

I agree with that. Hence my conceptual suggestion.

An old issue which is relevant is #65. There I linked a deep note example where the link between output and code is made visible through a light border.

Ohhh, had completely forgot about that one :o Here's a quick version of it hacked together:

The background color of the code is almost too light now. It's hard to see the difference between the white and gray.

HugoGranstrom · 2023-03-08T19:34:21Z

Here it is with approximately the same background color as deepnotes:

And here with the same border-color:

The padding can still be improved though, it's a bit squeezed right now:

HugoGranstrom · 2023-03-09T06:44:24Z

@dlesnoff sorry for going off-topic and discussing other stuff here as well. For this PR I think it would suffice if you remove the samp tag from the output (and the style we added for it) and then we'll merge this and continue with redesigning it in a separate PR.

dlesnoff · 2023-03-09T08:38:30Z

In which file do you add samp to the output? Le jeu. 9 mars 2023, 07:44, Hugo Granström ***@***.***> a écrit :

…

@dlesnoff <https://github.com/dlesnoff> sorry for going off-topic and discussing other stuff here as well. For this PR I think it would suffice if you remove the samp tag from the output (and the style we added for it) and then we'll merge this and continue with redesigning it in a separate PR. — Reply to this email directly, view it on GitHub <#185 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANDHQOE6YVD23CHKPY2VBUTW3F35HANCNFSM6AAAAAAVQJJBWY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

HugoGranstrom · 2023-03-09T08:41:05Z

Line 20 in renders.nim:

  doc.partials["nbCodeOutput"] = """{{#output}}<pre class="nb-output"><samp>{{output}}</samp></pre>{{/output}}"""

dlesnoff · 2023-03-09T09:35:18Z

I made the changes. I really like the one with a thick border, but it would help to see in context: what if the code is particularly large (and the border goes off-screen)? what if there are numerous code blocks?

HugoGranstrom · 2023-03-09T12:05:03Z

Ah right, we have to fix the tests now that we have changed the produced HTML. In tests/trender.nim you should remove the samp tags from the expected output of the tests that fail. Run nimble test locally to run the tests and check if it works

HugoGranstrom · 2023-03-09T12:06:39Z

And yes we need view the redesigned code blocks in a larger context before deciding the final design. I think something in-between the light and dark borders would be nice.

pietroppeter · 2023-03-09T15:39:40Z

thanks @dlesnoff for working on this and sorry for all the noise in the discussion!

I am fine with the current changes but noticed a couple of things:

is this PR really fixing CSS padding is too high for Code blocks #184 ? I think it is fixing code output block (which was discussed in CSS padding is too high for Code blocks #184) but not appearance of inline code (at the moment). I am fine with merging this and having a separate PR to work on padding of inline code blocks (also removal of normalize.css as far as I understand is out of scope here).
I noticed in netlify preview a double horizontal rule before the footer (see screenshot below). not sure if this is due to this PR (it is not an issue present in currently deploy site), it might be because of water.css version 2 fix? it is weird

double hr:

HugoGranstrom · 2023-03-09T16:12:18Z

We updated Water.css precisely because it solved #184 and reduced the padding of inline code blocks? 😛 In combination with removing normalize.css that is, but seems like we haven't done that here. I thought we did it, must be misremembering 🤔 So that's another thing that has to be done in this PR.

HugoGranstrom · 2023-03-09T16:17:17Z

So the remaining things TODO in this PR are:

Remove normalize.css in themes.nim
Fix tests in tests/trenders.nim

dlesnoff · 2023-03-10T08:48:20Z

sorry for going off-topic and discussing other stuff here as well.

I do not mind. It seemed to me related to the PR.
I was preparing a presentation at a research seminar, so I was not very available this week, that is the reason why I was not very responsive.

HugoGranstrom · 2023-03-10T08:59:32Z

Thanks a lot, it looks good to me now and the preview looks nice again :D

dlesnoff · 2023-03-10T17:28:31Z

J'ai testé:

code {
  font-size: 1em;
}

et

code {
  font-size: 2em;
}

ainsi qu'inherit.
Quelque chose comme 1.3 ou 1.4em peut faire l'affaire, mais ça modifie à la fois le code inline et le code.
Effectivement, je préfererais que le code soit modifié indépendamment du code inline qui devrait toujours avoir la même taille que le texte.

HugoGranstrom · 2023-03-10T17:31:41Z

Thank god for Google Translate 😅 (easy mistake to do when you forget to context switch :))

Indeed, we should add a new class that we put on the <code> tags in code blocks. And then change the size only for that class.

dlesnoff · 2023-03-10T19:39:21Z

My train arrived at destination at that time, sorry for the language mixup. I am copy-pasting the original message above with the correct translation (and a few addition):

Previous message

I have tested the following css snippets:

code {
  font-size: 1em;
}

and

code {
  font-size: 2em;
}

as well as inherit as a value of font-size attribute. Something like 1.3 or 1.4em might do apart that it modifies both inline code and code blocks.
Indeed, I would prefer that code blocks are modified independently from inline code. Inline code should always have the same size as the text.

What is the name of the partial corresponding to code blocks ?

HugoGranstrom · 2023-03-10T20:02:09Z

No worries :)

It's on the line above the partial of the code output if I recall correctly.

pietroppeter · 2023-03-10T21:06:37Z

Btw we should also remove the <hr> inside the footer since this version of water.css adds a top border to the footer element (hence why the double line at the end)

HugoGranstrom · 2023-03-11T10:53:50Z

The normal text and the code uses different fonts with slightly different sizes. So even if both a 1em now, they don't appear to be as big (code is smaller). For me, they appear roughly equal when code size is set to 1.2em. Are you seeing the same on your screens or does it differ between devices? (I'm on Linux)

dlesnoff · 2023-03-11T11:20:02Z

It differs between two Linux devices. I don't see the same on my PC (Archlinux) and my laptop (Ubuntu). Using Firefox on both!

HugoGranstrom · 2023-03-11T11:25:15Z

😅

Might the screen size play into it perhaps? Maybe the normal text is defined in pixels while the code is in relative units. I haven't been able to find the text size of the normal text in the inspector :/

dlesnoff · 2023-03-11T11:28:34Z

No wait. I pushed the modifications on my blog online, and now the code appears too small on my Archlinux device too.
I think I had a confusion between versions of nimib due to a problem in nimble.
It seems like the current version of nimble does not select the latest installed version by default, this is weird.

dlesnoff · 2023-03-11T12:13:45Z

Nimble install does not install my working directory of nimib.

HugoGranstrom · 2023-03-11T12:19:35Z

Are you using nimble v0.14 perhaps? They changed so much about local installations, I'll stay on 0.13 as long as I can.

dlesnoff · 2023-03-11T12:36:44Z

I used nimble install nimib instead of nimble install. Yes I use nimble v0.14.

HugoGranstrom · 2023-03-11T12:39:47Z

I think the preview looks good now, the inline code doesn't overflow and the code blocks looks the same size as the normal text on both my laptop and phone 🚀

dlesnoff · 2023-03-11T12:49:07Z

The inline code does overflow for me for some reason.
I wonder if nimble install only installs main/master git branch. This would be weird.

HugoGranstrom · 2023-03-11T15:05:25Z

I wonder if nimble install only installs main/master git branch. This would be weird.

I wouldn't be suprised :P I guess you could do something like nimble install https://github.com/dlesnoff/nimib@updateWater if you want to sanity check that you are using the correct version (and do a nimble remove nimib first to remove all old ones)

pietroppeter · 2023-03-11T20:49:53Z

This is what I see from iOS/Safari looking at the preview:

Text, in-line code and code black seem they have 3 different sizes, so it does not come out great 🤨 not sure how to help on this though

pietroppeter · 2023-03-11T21:05:05Z

this is what I see on a desktop windows with Chrome browser:

in this case I do not perceive a big difference between text and code block. inline code keeps being smaller than the two.

HugoGranstrom · 2023-03-11T21:29:13Z

Text, in-line code and code black seem they have 3 different sizes, so it does not come out great raised_eyebrow not sure how to help on this though

Huh, yeah that doesn't look too good. The inline code is fine, we can't increase its size after all without overflowing so there is nothing to do about it. If the font-size increase, the box around it increases so it's impossible for the inline code to be as big as the normal text. But the code blocks are pretty big/the normal text is small.

I have done some checking on Firefox on my laptop and it seems the normal text gets the default text size 16px while we set the code size to 1.2em. So Safari on IOS might have some other default text size that makes them look different. So we could try and set our own default text size (on body?) to 1em or something so we know that we are working in the same units for all texts.

pietroppeter

Thanks for the work on this @dlesnoff, I think there are still issues on the font sizes but this PR definitely addresses the specific issues it targets so I would actually vote for merging this and open a separate issue for the font sizes.

HugoGranstrom · 2023-03-12T10:02:14Z

Agreed, and ideally someone who can reproduce the different font sizes should be the one to implement that PR so one doesn't have to wait for someone else just to know if it works or not.

dlesnoff · 2023-03-13T13:03:09Z

Fine by me, I will not be working on fixing the font sizes.
Otherwise, I might let the PR on draft mode for months with the incoming work on the Algorithms.
I can't test the font without pushing/tagging and looking at the netlify preview.

You have accepted the changes but you have not merged the PR? Do you wait for the PR that will fix the font sizes?

pietroppeter · 2023-03-13T13:08:54Z

I think we were just waiting to be sure you did not plan to add something else. Thanks again, merged!

pietroppeter · 2023-03-13T13:09:52Z

And I guess the hint is that I will take the PR on font sizes, will open an issue and assign it later. ;)

HugoGranstrom · 2023-03-13T13:12:25Z

We just waited for confirmation by you I guess before we merged it 😉

Update water.css and fix code output

ddc8bb7

Remove samp in output and in nimib CSS

ecb7bab

dlesnoff added 2 commits March 10, 2023 09:45

Remove normalize.css

b47d15a

Remove samp from tests

7b88f25

Force code size to be identical to text and remove extra line in footer

cebcd12

Rescale code blocks font size

525bc94

pietroppeter approved these changes Mar 12, 2023

View reviewed changes

pietroppeter merged commit a5dc283 into pietroppeter:main Mar 13, 2023

dlesnoff deleted the updateWater branch March 13, 2023 14:20

pietroppeter mentioned this pull request Mar 27, 2023

fix font sizes on some os/browsers #197

Open

HugoGranstrom mentioned this pull request Jul 10, 2023

improve rendering of code output #65

Open

Update water.css and fix code output #185

Update water.css and fix code output #185

Conversation

dlesnoff commented Mar 5, 2023

HugoGranstrom commented Mar 5, 2023 • edited Loading

pietroppeter commented Mar 6, 2023

HugoGranstrom commented Mar 6, 2023

pietroppeter commented Mar 6, 2023

pietroppeter commented Mar 6, 2023

pietroppeter commented Mar 6, 2023

dlesnoff commented Mar 6, 2023 via email

HugoGranstrom commented Mar 6, 2023

pietroppeter commented Mar 7, 2023

HugoGranstrom commented Mar 8, 2023

HugoGranstrom commented Mar 8, 2023

HugoGranstrom commented Mar 8, 2023

pietroppeter commented Mar 8, 2023

HugoGranstrom commented Mar 8, 2023 • edited Loading

HugoGranstrom commented Mar 8, 2023 • edited Loading

HugoGranstrom commented Mar 9, 2023

dlesnoff commented Mar 9, 2023 via email

HugoGranstrom commented Mar 9, 2023

dlesnoff commented Mar 9, 2023

HugoGranstrom commented Mar 9, 2023

HugoGranstrom commented Mar 9, 2023

pietroppeter commented Mar 9, 2023

HugoGranstrom commented Mar 9, 2023 • edited Loading

HugoGranstrom commented Mar 9, 2023 • edited Loading

dlesnoff commented Mar 10, 2023 • edited Loading

HugoGranstrom commented Mar 10, 2023

dlesnoff commented Mar 10, 2023

HugoGranstrom commented Mar 10, 2023

dlesnoff commented Mar 10, 2023

Previous message

HugoGranstrom commented Mar 10, 2023

pietroppeter commented Mar 10, 2023

HugoGranstrom commented Mar 11, 2023

dlesnoff commented Mar 11, 2023

HugoGranstrom commented Mar 11, 2023

dlesnoff commented Mar 11, 2023

dlesnoff commented Mar 11, 2023

HugoGranstrom commented Mar 11, 2023

dlesnoff commented Mar 11, 2023

HugoGranstrom commented Mar 11, 2023

dlesnoff commented Mar 11, 2023 • edited Loading

HugoGranstrom commented Mar 11, 2023

pietroppeter commented Mar 11, 2023 • edited Loading

pietroppeter commented Mar 11, 2023

HugoGranstrom commented Mar 11, 2023

pietroppeter left a comment

Choose a reason for hiding this comment

HugoGranstrom commented Mar 12, 2023

dlesnoff commented Mar 13, 2023

pietroppeter commented Mar 13, 2023

pietroppeter commented Mar 13, 2023

HugoGranstrom commented Mar 13, 2023

HugoGranstrom commented Mar 5, 2023 •

edited

Loading

HugoGranstrom commented Mar 8, 2023 •

edited

Loading

HugoGranstrom commented Mar 8, 2023 •

edited

Loading

HugoGranstrom commented Mar 9, 2023 •

edited

Loading

HugoGranstrom commented Mar 9, 2023 •

edited

Loading

dlesnoff commented Mar 10, 2023 •

edited

Loading

dlesnoff commented Mar 11, 2023 •

edited

Loading

pietroppeter commented Mar 11, 2023 •

edited

Loading