Skip to content

Notebook cell styling (nbsphinx only) #2187

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

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

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented Apr 11, 2025

This is an implementation of Isabela's designs for notebook cell styling.

The pull request has mainly two parts:

  1. A bunch of CSS rules that apply on top of nbsphinx's style rules. In many cases, I had to override nbsphinx's rules to achieve the desired look.
  2. An example notebook copied from nbsphinx's documentation that demonstrates lots of different code cells. This gives us some assurances that the styles are working as we intend and will help us spot errors in the future.

Caution

The approach taken in this PR to overriding nbsphinx's style rules is not long-term sustainable. We will need to find a way to work with nbsphinx to either upstream these styles or to find a way to better scope the CSS rules.

Known differences from the design

  • In the design, the labels for the code cells follow a format like "In 1" and "Out 1", but these labels are generated by nbsphinx so they are not easy to completely override with CSS. So in this pull request they follow a format like "In [1]" and "Out [1]" with brackets.

How to test this pull request

To test on the preview build:

  1. Go to the code cells page in the preview build.
  2. Inspect it visually.
  3. Also press the tab key to move about the page and see how the focus ring looks.
  4. Do the same for the pydata notebook

To test locally, build the docs locally and check the same pages.

@gabalafou gabalafou changed the title prettier Notebook cell styling Apr 11, 2025
@gabalafou gabalafou marked this pull request as ready for review April 25, 2025 17:11
Copy link
Contributor

@isabela-pf isabela-pf left a comment

Choose a reason for hiding this comment

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

This looks so good! I'm happy to be able to say nothing I saw on the preview page surprised me. Visually, this implementation follows the previously discussed designs and fits in with the visuals surrounding it. It was especially helpful to see it in use on so many notebook output types for review.

In the design, the labels for the code cells follow a format like "In 1" and "Out 1", but these labels are generated by nbsphinx so they are not easy to completely override with CSS. So in this pull request they follow a format like "In [1]" and "Out [1]" with brackets.

I consider this an acceptable break from the design, and really an oversight on my part to not catch the way nbsphinx was doing it in the first place. Thanks for calling this out for clarity regardless.

Other review notes:

  • This PR doesn't interrupt any responsive page behaviors, nor does it make any new responsive problems as far as I can tell.
  • I can navigate through the page contents with my keyboard the same way I could before. I'm not finding any changes in behavior with keyboard only-navigation.
  • I am not finding any changes in screen reader (VoiceOver) behavior with this PR. Everything appears to operate and read the same.
  • The visual changes this PR makes matches visuals in the existing theme. I wasn't concerned about this changing based on previous discussions, but I do want to vouch it was done properly.

Design-wise, I don't have any changes to request. Thanks for reaching out for a review.

@trallard trallard self-requested a review April 28, 2025 16:04
@gabalafou gabalafou changed the title Notebook cell styling Notebook cell styling (nbsphinx only) Apr 29, 2025
Copy link
Collaborator Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

Finished self review. I feel pretty good about this.

One thing to consider is whether we should make it possible to disable these styles with a configuration variable.

- [email protected]
- [email protected]
- stylelint
- stylelint-scss
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed these lines because I was getting indecipherable error messages when trying to commit scss files with lint errors. After these changes, I got intelligible error messages.

"metadata": {
"nbsphinx": "hidden"
},
"source": [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since most of this notebook is copied from the nbsphinx repo, I copied the MIT license from that repo and added it here in a cell that's hidden from nbsphinx—that way, the notice appears in the source code or any notebook application, but is not shown on the docs site.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These images are needed by the code-cells notebook

"mpg_df = pd.read_csv(mpg_csv, index_col=0)\n",
"mpg_df"
]
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this table because I wanted to see if the table-layout: auto rule would have any effect. (It doesn't.)

Comment on lines 61 to 89
// Override nbsphinx's colors for notebook cell prompts because they do not have
// sufficient contrast. Colors chosen from accessible-pygments
// a11y-high-contrast-{light,dark} themes.
// Override nbsphinx's styles with our own <pre> tag styles
div.nbinput.container div.input_area div[class*="highlight"] > pre,
div.nboutput.container div.output_area div[class*="highlight"] > pre {
@extend %pre-style;

// Notebook cell input line number. Replace nbsphinx's low contrast blue with
// higher contrast blues.
.nbinput.container .prompt pre {
html[data-theme="light"] & {
// Copied from accessible-pygments [a11y-high-contrast-light](https://github.com/Quansight-Labs/accessible-pygments/tree/main/a11y_pygments/a11y_high_contrast_light)
color: #005b82;
margin: 0;
}

html[data-theme="dark"] & {
// Copied from accessible-pygments [a11y-high-contrast-dark](https://github.com/Quansight-Labs/accessible-pygments/tree/main/a11y_pygments/a11y_high_contrast_dark)
color: #00e0e0;
// Last container styles
div.container.nblast {
margin-bottom: $top-and-bottom-container-space;

// Override nbsphinx's padding rule. Let the whitespace be controlled by the
// margin-bottom rule above. This is important because if we create
// whitespace with padding instead of margin, then we create a misalignment
// between the vertical left bar and the bottom of the notebook cell.
padding-bottom: 0;
}
}

// Notebook cell output line number. Replace nbsphinx's low contrast red with
// higher contrast red / orange.
.nboutput.container .prompt pre {
html[data-theme="light"] & {
// Copied from accessible-pygments [a11y-high-contrast-light](https://github.com/Quansight-Labs/accessible-pygments/tree/main/a11y_pygments/a11y_high_contrast_light)
color: #a12236;
}
// Nest under .bd-article-container so that selectors have higher specificity
// and can therefore override the rules in the nbphinx stylesheet.
.bd-article-container {
@include _nbsphinx-restyle;

.nboutput {
.output_area {
&.rendered_html,
.jp-RenderedHTMLCommon {
// pandas
table.dataframe {
@include table-colors;

tbody {
tr:hover {
background-color: var(--pst-color-table-row-hover-bg);
}
}
}
}

html[data-theme="dark"] & {
// Copied from accessible-pygments [a11y-high-contrast-dark](https://github.com/Quansight-Labs/accessible-pygments/tree/main/a11y_pygments/a11y_high_contrast_dark)
color: #ffa07a;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed these lines (61-89) because in Isabela's design, the cell label text, In [N] and Out [N], is just the same color as the surrounding documentation text.

tbody {
tr:hover {
background-color: var(--pst-color-table-row-hover-bg);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mostly yanked the above lines (12-36) and pasted them further down the file.

However, the margin rule was no longer needed because the other style rules added take care of focus ring issues.


div.input_area {
border: none; // border taken care of by <pre> styles elsewhere in the theme
overflow: visible;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think all of the overflow: visible rules added in this PR were added to make the focus ring visible.

A safer, but more complex alternative would be to carefully add focus-ring-width padding to focusable elements. However, I have observed that whatever gets rendered into the input or output areas is usually rendered within some kind of container that has overflow: auto rules. In other words, I haven't seen any cases where the content rendered into the output or input areas overflows its parent (except for a few specific HTML widget cases that I handle separately).

Furthermore, I tried going down the route of adding padding to create space for the focus ring, and it really is quite a bit more complex. If you want the stuff in the input and output areas to be left-aligned with the labels (as in Isabela's designs), then you have to start carefully subtracting padding from parent elements wherever you add padding to child elements. Furthermore, as you will see further down, the way I fixed padding-related alignment issues was to set padding to zero using the same selectors that nbsphinx uses in its CSS. When you set padding to zero, you don't have to worry whether you're applying the rule to both parent and child. But when you add padding to create space in which to draw the focus ring, then you have to know whether the parent or child is getting focus, and the way this currently works is not consistent. Sometimes the parent element gets focus, for example the div.output_area containing a wide Pandas data table. Sometimes the child gets focus, for example the pre element inside of the div.output_area in the case of a plain text output.

So if you want to add focus-ring-width padding to div.output_area you have to make sure it does not contain any of those elements that are focusable and that also need focus-ring-width padding. That would mean a selector that looks like:

div.output_area:not(:has(pre, ...))

And I just think this is a very messy way to write CSS, and may not even work well across all browsers.

So I think it's worth taking the risk of setting overflow: visible in several places.

Comment on lines +141 to +147
// Override our own style rule in _math.scss
div.math {
display: block;
}

span.math {
display: inline;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will open a follow-up PR to better handle math content both within notebooks and outside of notebooks. That way we won't need to override our own rules.

&.rendered_html:not(:has(table.dataframe)),
// ipywidgets
.widget-subarea {
@include cell-output-background;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These lines (41-46) are just moved down, not actually deleted.

}
// Nest under .bd-article-container so that selectors have higher specificity
// and can therefore override the rules in the nbphinx stylesheet.
.bd-article-container {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some of these rules used to be nested under .bd-content. But as far as I can tell, notebook content will always be rendered within .bd-article-container, so this is the better selector to use for nesting.

@gabalafou gabalafou added impact: high Something that is relevant to nearly all users DO NOT MERGE There is something blocking merging this PR labels May 6, 2025
@gabalafou
Copy link
Collaborator Author

Labeling this as "do not merge" because it can be reviewed right now but it should not be merged until after 0.17 is released

@gabalafou gabalafou added this to the 0.18.0 milestone May 6, 2025
@gabalafou gabalafou added the tag: design Items related to design tasks or improvements label May 6, 2025
Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

this is looking pretty good! nice work @isabela-pf and @gabalafou. Comments below on quirks that I noticed.

Comment on lines +142 to +144
"The standard error stream is highlighted and displayed just below the code cell.\n",
"The standard output stream comes afterwards (with no special highlighting).\n",
"Finally, the \"normal\" output is displayed."
Copy link
Collaborator

Choose a reason for hiding this comment

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

as mentioned in the note box below, this doesn't necessarily reflect the order in the rendered doc (where stdout precedes stderr): https://pydata-sphinx-theme--2187.org.readthedocs.build/en/2187/examples/code-cells.html

Can we generalize the language here to say "normal output comes last" and update the note to say "when stdout/stderr are both present, which one comes first can vary by kernel"?

Comment on lines +275 to +283
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"Image(url=\"https://jupyter.org/assets/homepage/main-logo.svg\")"
]
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems redundant; what does this cell demonstrate that is different from the one 2 cells before it (the first python logo, without embed=True)? Is .png vs .svg the point?

"\n",
"\n",
"Latex(\n",
" r\"This is a \\LaTeX{} equation \"\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

\\LaTeX{} isn't rendering correctly in the output

Comment on lines +353 to +354
"<div class=\"alert alert-info\">\n",
"\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe better? Or was this meant to intentionally show a no-title alert box?

Suggested change
"<div class=\"alert alert-info\">\n",
"\n",
"<div class=\"alert alert-info\">\n",
"\n",
"Note\n",
"\n",

"outputs": [],
"source": [
"fig, ax = plt.subplots(figsize=[6, 3])\n",
"ax.plot([4, 9, 7, 20, 6, 33, 13, 23, 16, 62, 8]);"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this renders as a .png in the built docs, identical to the .png in the following cell's output.

Comment on lines +504 to +510
"source": [
"df = pd.DataFrame(\n",
" np.random.randint(0, 100, size=[10, 4]),\n",
" columns=[r\"$\\alpha$\", r\"$\\beta$\", r\"$\\gamma$\", r\"$\\delta$\"],\n",
")\n",
"df"
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

output is illegible in dark mode:

Screenshot_2025-05-08_11-24-49

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, dark mode stuff is tricky, but I will take another look into it and see if I can come up with a reasonable solution.

Comment on lines +537 to +553
"source": [
"a = {}\n",
"for name in [\"t\", \"q\"]:\n",
" da = xr.DataArray(\n",
" [\n",
" [[11, 12, 13], [21, 22, 23], [31, 32, 33]],\n",
" [[14, 15, 16], [24, 25, 26], [34, 35, 36]],\n",
" ],\n",
" coords={\"level\": [\"500\", \"700\"], \"x\": [1, 2, 3], \"y\": [4, 5, 6]},\n",
" name=name,\n",
" attrs={\"param\": name},\n",
" )\n",
" a[name] = da\n",
"\n",
"ds = xr.Dataset(a, attrs={\"title\": \"Demo dataset\", \"year\": 2024})\n",
"ds"
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

output is illegible in dark mode, though for xarray content this is a known bug, not a regression (#2189)

Comment on lines +706 to +714
"link = w.IntSlider(description=\"link\")\n",
"w.link((slider, \"value\"), (link, \"value\"))\n",
"jslink = w.IntSlider(description=\"jslink\")\n",
"w.jslink((slider, \"value\"), (jslink, \"value\"))\n",
"dlink = w.IntSlider(description=\"dlink\")\n",
"w.dlink((slider, \"value\"), (dlink, \"value\"))\n",
"jsdlink = w.IntSlider(description=\"jsdlink\")\n",
"w.jsdlink((slider, \"value\"), (jsdlink, \"value\"))\n",
"w.VBox([link, jslink, dlink, jsdlink])"
Copy link
Collaborator

Choose a reason for hiding this comment

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

the labels "link", "dlink", "jslink", and "jsdlink" won't communicate the 4 scenarios clearly to many readers. Can we change to something like:

  • kernel vs javascript
  • bidirectional vs oneway

and also specify that what these are (bi/uni)directionally linking to is slider from the prior cell?

Comment on lines +722 to +731
"source": [
"tabs = w.Tab()\n",
"for idx, obj in enumerate([df, fig, eq, i, md, slider]):\n",
" out = w.Output()\n",
" with out:\n",
" display(obj)\n",
" tabs.children += (out,)\n",
" tabs.set_title(idx, obj.__class__.__name__)\n",
"tabs"
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. math isn't correctly rendered in dataframe colnames, nor in the dedicated math tab.

  2. Markdown text is nearly illegible in dark mode:

    Screenshot 2025-05-08 at 11-35-36 Code Cells — PyData Theme 0 16 2dev0 documentation

  3. contrast on widget slider handle border is pretty subtle

    Screenshot 2025-05-08 at 11-36-14 Code Cells — PyData Theme 0 16 2dev0 documentation

Comment on lines +182 to +194
{
"cell_type": "code",
"execution_count": null,
"metadata": {
"tags": [
"raises-exception"
]
},
"outputs": [],
"source": [
"traceback # NOQA: F821"
]
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's odd that this cell gets the green "success" color for its left-border line. I would have expected orange/red. 🤔

Copy link
Collaborator Author

@gabalafou gabalafou May 9, 2025

Choose a reason for hiding this comment

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

This is a very good point. I wonder if we should use a different color that doesn't have semantic associations with success or failure. Maybe a gray border would be better? @isabela-pf, what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's a screenshot for reference:

@gabalafou
Copy link
Collaborator Author

@drammock, thank you so much for this feedback!

A lot of the things you caught have something to do with the original Code Cells notebook and nbsphinx itself. Almost all of the problems that you point out exist on the corresponding nbsphinx docs page for Code Cells (except for the dark mode stuff because the nbsphinx docs site does not support dark mode).

Some of these things I can fix on our docs by modifying the Code Cells notebook (such as changing the prose to make the document clearer). Some of these things will be beyond my ability to fix in this PR because they would require me to patch nbsphinx (or further upstream).

I think your comments raise an important point that I didn't fully appreciate when I copied this notebook from nbsphinx to our own documentation, which is that once it is on our docs site, we become in some way responsible for whatever shortcomings exist in the document. I was thinking about this page less in terms of documentation and more as just a place to check how our theme combines with nbsphinx. But maybe the docs site is not the appropriate place for that. Do you think it would make sense to create a separate site that isn't aimed at end users? Not a doc site but a demo site?

@drammock
Copy link
Collaborator

drammock commented May 9, 2025

I think docs/examples is a fine place for content like this (many users won't use nbsphinx, but for those who do, they should be able to easily see and assess how well we interoperate with it). I agree that by incorporating it here we take some degree of ownership, and I think that the right way forward is to modify the content to make clear what we can and can't fix (i.e., make the example a bit more self-documenting). In other words, more interleaved prose that points out the shortcomings and the barriers (big or small) to overcoming them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE There is something blocking merging this PR impact: high Something that is relevant to nearly all users tag: design Items related to design tasks or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants