search

spatialaudio / nbsphinx

:ledger: Sphinx source parser for Jupyter notebooks
https://nbsphinx.readthedocs.io/
MIT License
451 stars 130 forks source link

Update explanation regarding Raw cells #632

Closed 1kastner closed 2 years ago

1kastner commented 2 years ago
1kastner commented 2 years ago

Fixes #625

1kastner commented 2 years ago

The result of the runner is not reproducible for me on my local machine even though I copied the command from the action output at https://github.com/spatialaudio/nbsphinx/runs/4773390871?check_suite_focus=true

1kastner commented 2 years ago

Duplicate substitution definition name: "Steps for converting cells to Raw formats in Jupyter".

I need to further research how that can be solved best.

mgeier commented 2 years ago

The error might not be reproducible due to different Pandoc versions.

The solution should be to simply change one of the Steps for converting cells to Raw formats in Jupyter, which shouldn't be the same anyway.

1kastner commented 2 years ago

The error might not be reproducible due to different Pandoc versions.

The solution should be to simply change one of the Steps for converting cells to Raw formats in Jupyter, which shouldn't be the same anyway.

That sounds reasonable. I added "Notebook" or "Lab" depending on the UI.

1kastner commented 2 years ago

I hope @mgeier I could fix all the issues you spotted.

mgeier commented 2 years ago

Yes, thanks!

However, I'm not sure if scaling the images to 100% width is the right thing to do.

This might actually be a Sphinx theme problem ... many themes automatically scale images to 100% width, but that doesn't mean it's the best thing to do.

The problem is that on small screens (e.g. smartphone) the screenshot might become unreadable. Granted, users can zoom in, but I think that's kinda complicated.

Also, the scaled version looks more ugly than the unscaled one.

What do you think about making the images scrollable?

I've implemented this in the insipid theme: https://github.com/mgeier/insipid-sphinx-theme/pull/66

1kastner commented 2 years ago

Making the image scrollable sounds acceptable to me. You previously mentioned that the JupyterLab image was clipped and when I had a closer look, I saw that the Jupyter Notebook image was slightly clipped in the prior version, too. And having clipped images in the documentation is something I would prefer to avoid.

EDIT: I had a look at how the scrollable images are achieved at https://raw.githubusercontent.com/mgeier/insipid-sphinx-theme/master/doc/showcase/images.rst and I see that it is part of the theme and not up to me. Which leads me to the question following question:

What do you think about making the images scrollable?

Well, I would not mind (it is not beautiful either but on small screens a viable alternative). But I could not see at https://github.com/mgeier-forks/insipid-sphinx-theme/blob/scroll-large-images-and-tables/doc/showcase/images.rst or https://raw.githubusercontent.com/mgeier/insipid-sphinx-theme/master/doc/showcase/images.rst how one controls for that - especially as there are plenty of themes that are supported by nbsphinx and not only insipid.

I would rather like to see this pull request merged earlier and move your totally valid concerns of not readable screenshots to a next working package. Would that be an acceptable procedure for you?

mgeier commented 2 years ago

I had a little setback: https://github.com/mgeier/insipid-sphinx-theme/pull/66 doesn't seem to work as intended.

I had to come up with an alternative (https://github.com/mgeier/insipid-sphinx-theme/pull/68), which isn't perfect either, but it is much closer to other Sphinx themes.

What do you think about that?

I would rather like to see this pull request merged earlier and move your totally valid concerns of not readable screenshots to a next working package. Would that be an acceptable procedure for you?

I would prefer to fix the theme issue first. Because depending on that, this PR might have to be changed.

I think the max-width: 100% approach is the way to go for now, even though it has its disadvantages.

If we decide to do this, it doesn't make sense to set 100% in this PR. In this case, the old Markdown links can be restored because we don't need the special <img> handling.

especially as there are plenty of themes that are supported by nbsphinx and not only insipid.

That's a good point. With https://github.com/mgeier/insipid-sphinx-theme/pull/68, the behavior of insipid should get closer to the other themes.

The rendering in different themes can be checked reasonably quickly with the https://github.com/spatialaudio/nbsphinx/blob/master/theme_comparison.py script.

1kastner commented 2 years ago

The old markdown links have been restored. I would proceed this way:

  1. I will go through all themes as they are generated by https://github.com/spatialaudio/nbsphinx/blob/master/theme_comparison.py - not more and not less.
  2. I will have a look how they treat the screenshots. If the screenshots are clipped in my browser, then I will check whether max-width: 100% is set in the theme or not.
  3. In case it is currently missing, I would add max-width: 100% to the image tag (most likely defined in some CSS file) to avoid clipped graphics for now.

First of all, is that the workflow you intended?

I followed the steps of https://github.com/spatialaudio/nbsphinx/blob/efd08d8f777dcf7f435625b4a5726e240b5e2937/CONTRIBUTING.rst#building-themes and I encountered an issue that I opened at #634.

EDIT:

After #634 has been resolved, I could check the themes as described above. These themes already have a max-width: 100% (or width: 100%) for images or the content area is so wide that it is irrelevant:

The themes currently do not have a max-width: 100% for their images...

As the later themes are maintained by separate developers, the max-width must be passed through in some other way, e.g. by having additional CSS definitions that are added to the generation process.

mgeier commented 2 years ago

Thanks for testing all the themes, very interesting!

... and instead the image is clipped

What do you mean by "clipped"? Do you mean that the page has to be horizontally scrolled to be able to see the right edge of the image?

I have just merged https://github.com/mgeier/insipid-sphinx-theme/pull/68, which means that the insipid theme should now (once a release has been made) behave like most other themes with regards to shrinking large images.

As the later themes are maintained by separate developers, the max-width must be passed through in some other way, e.g. by having additional CSS definitions that are added to the generation process.

I think it is the intention of the theme authors not to scale the images, so I would simply leave it as it is.

However, if you want to improve the overall quality of this PR, you could create the screenshots again with a bit smaller resolution. We can't make it perfect for all themes, but we could choose a width that's displayed in most themes without shrinking. And BTW, if you decide to do the screenshots again, you could try to close JupyterLab's left sidebar and make the right sidebar a bit narrower, which would leave more room for the middle column.

1kastner commented 2 years ago

What do you mean by "clipped"? Do you mean that the page has to be horizontally scrolled to be able to see the right edge of the image?

I use a quite new version of Firefox and there is no scrollbar. Instead, the right part of the image is just not visable. Here is the screenshot of the screenshot as it is displayed to me in my browser (looking at /build/raw-cells.html):

grafik

However, if you want to improve the overall quality of this PR, you could create the screenshots again with a bit smaller resolution.

I might try to resize the next screenshot slightly. I hope the quality does not suffer too much.

EDIT: Now I found the scroll bar at the end of the HTML page. I misunderstood you and I believed the image itself would receive a scroll bar. I was not expecting a scroll bar there, sorry!

1kastner commented 2 years ago

I added the resized images and used them in the Jupyter Notebook. The resized version was created with the following code:

<nbspinx-project-dir>/doc/images$ convert -resize 65% raw_cells_jupyter_notebook.png raw_cells_jupyter_notebook_resized.png
<nbspinx-project-dir>/doc/images$ convert -resize 65% raw_cells_jupyterlab.png raw_cells_jupyterlab_resized.png

The original images now open when you click on the resized images. I believe that would come quite handy for people who are visually impaired and who would have troubles using the resized images. Moreover, I came up with the 65% percent just by guess-work and in the future, as the insipid theme might evolve, so might its default pixel width change?

I hope with these last updates on the pull request I could fulfill all requested changes to your full satisfaction.

mgeier commented 2 years ago

Thanks for the update!

The original images now open when you click on the resized images. I believe that would come quite handy for people who are visually impaired and who would have troubles using the resized images.

I don't know much about the accessibility of screenshots, but I guess it's good to have the pixel-exact screenshot because the re-scaled image might be a bit blurry.

Moreover, I came up with the 65% percent just by guess-work

But how did you come up with the width of the original screenshot?

Why did you not make that 65% narrower?

as the insipid theme might evolve, so might its default pixel width change?

Sure, the default width might change. I don't expect it to get narrower, but there are no guarantees.

I hope with these last updates on the pull request I could fulfill all requested changes to your full satisfaction.

Well, I'm fine with them, and if you want I can merge them as is.

But I wouldn't call it "full satisfaction" ...

I think the two versions of each screenshot are overkill.

Why not make the original screenshot narrower, so it doesn't have to be scaled in the first place?

This was the situation of the previous screenshot and it looked very crisp.

1kastner commented 2 years ago

Why not make the original screenshot narrower, so it doesn't have to be scaled in the first place?

I just tried to re-take the screenshots in the narrower way. For the Jupyter Notebook UI, this is less of an issue. When I try to make the crisp screenshot of JupyterLab, the UI elements start to arrange in a way it usually does not when people use JupyterLab.

This is what I would call the "normal" arrangement of the user interface but the screenshot is too wide: raw_cells_jupyterlab

For me, this narrow arrangement of the user interface is already distorted even though it is ~800px and we are not even close to the 700px to crack (that is the width of the 65%-resized images). I had to manually enable the second toolbar so that the cell type dropdown menu was even visible. raw_cells_jupyterlab

By default, the left menu has a fixed width, so I can only make the displayed Jupyter Notebook narrower. So here I cracked to border of 700 px. Only now we can use the screenshot in the theme without resizing. In this case, however, the content of the screenshot between Jupyter Notebook and JupyterLab deviate. I guess the JupyterLab UI was never designed for such narrow images.

raw_cells_jupyterlab

For me, this is a rather unusual way of how the JupyterLab UI is arranged and I am afraid that users might be confused. Furthermore, the two screenshots now display different content which I also previously tried to avoid. But I guess for every point there are pro and con arguments and I can definitively understand yours as well. If you feel that this is preferable, I will re-arrange the pull request.

mgeier commented 2 years ago

That's strange. When I tried it, the buttons were not messed up:

image

Can you make the fonts smaller (by pressing Ctrl-minus or something)?

I find it strange that the right sidebar has a maximum width that's IMHO unnecessarily large. That should probably be reported, but I don't have enough energy to do that. I already have a litany of JupyterLab issues I would like to report some day ...

I guess the JupyterLab UI was never designed for such narrow images.

I guess not. But people are talking about it, e.g. https://github.com/jupyterlab/jupyterlab/issues/3275#issuecomment-618470517

If you feel that this is preferable, I will re-arrange the pull request.

I'll leave it up to you.

1kastner commented 2 years ago

In the screenshot you show, the dropdown to select "Raw cell" is missing - something you pinpointed in one of my previous screenshots.

1kastner commented 2 years ago

I updated the font size as you suggested, removed the resized image, and made a small fix. With my local pandoc installation everything is fine again and so it does to me in the auto-triggered readthedocs. I guess there is no homework left and things are set now.

mgeier commented 2 years ago

Thanks for your patience!

I think it is much better now.

In the screenshot you show, the dropdown to select "Raw cell" is missing - something you pinpointed in one of my previous screenshots.

Yeah, sorry, I missed that!