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

Implement @smeragoel's table designs #1757

Merged
merged 7 commits into from
May 13, 2024
Merged

Conversation

@trallard trallard added the tag: accessibility Issues related to accessibility issues or efforts label Apr 5, 2024
@gabalafou
Copy link
Collaborator Author

The preview build has been checked by @smeragoel. This PR is now ready for review.

@gabalafou gabalafou marked this pull request as ready for review April 12, 2024 13:41
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.

self review

),
"data-table-inner-border": (
"light": #{map-deep-get($color-palette, "gray", "200")},
"dark": #364150,
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 tried using one of the theme colors, but none of them looked as good to me as the one that @smeragoel picked out, which is labeled in Figma as foreground/light/muted-old.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This works for me. IIRC, I had to slightly tweak a couple of colours as I worked on the implementation of the colour palette to meet contrast requirements here and there and this was one of the colours I ended up having to modify 👍🏽 as long as we have this as a global variable and not an ad hoc colour in a stylesheet I think this is good.

"cell_type": "markdown",
"metadata": {},
"source": [
"## IPyWidget"
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 edited this example notebook to add a pandas data table inside of an ipywidget to help us catch issues like #1740 in the future.

@trallard
Copy link
Collaborator

Had a quick nosey and I have a couple questions

  1. Non-data tables will be styled in a separate PR correct?
  2. Since the tables in widgets seem to inherit/use the same style was it intentional that the div/tabbed component in which these are embedded remain light grey in dark mode? Or is this an unintended thing?

@gabalafou
Copy link
Collaborator Author

gabalafou commented Apr 15, 2024

Replies to above questions:

  1. I think that's a good plan to keep this PR moving along.
  2. I think you're referring to when the Pandas data table is put inside the example tabbed ipywidget. We get a set of layers that goes (back to front) black > gray > white > black, as shown in the following screenshot:
    .

I admit it doesn't look great but it is intentional. I applied the cell-output-background mixin to the ipywidget container (.widget-subarea) for the same reason it is applied to other notebook outputs—namely, neither those outputs nor ipywidgets have been styled for dark themes.

If you don't apply the cell-output-background mixin with the light gray background, then you're not going to see the black text "Hello" because ipywidgets isn't styled for dark mode—it assumes a light background.

There's really no way around this. If we want to follow the existing dark/light mode conventions in the stylesheets and we want to apply these new styles to Pandas data tables that appear inside of ipywidgets, then we are going to end up with this dark-light-dark layering in dark mode. There are only three ways I see around it:

  1. Don't apply the data tables mixin to ipywidgets, which would mean our new styles would not get applied to Pandas data tables inside of ipywidgets
  2. Write two versions of the mixin, one light, one dark, rather than relying on CSS vars that vary on mode. This is possible I think but it goes against the pattern of how dark mode is implemented in other places in our stylesheet code.
  3. Try to add our own styles to ipywidgets so they look good in dark mode. My guess is we could write a set of rules that work in most but not all cases. This would be a band-aid solution until ipywidgets supports dark mode. If ipywidgets supported dark mode, we wouldn't have this dark-light-dark layering issue at all.

@gabalafou gabalafou changed the title Implement @smeragoel's data table designs Implement @smeragoel's table designs Apr 18, 2024
@gabalafou
Copy link
Collaborator Author

Failed checks unrelated, see #1772

@gabalafou gabalafou requested a review from trallard April 18, 2024 21:05
@gabalafou
Copy link
Collaborator Author

Not sure about the failing codecov/project check...

),
"data-table-inner-border": (
"light": #{map-deep-get($color-palette, "gray", "200")},
"dark": #364150,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works for me. IIRC, I had to slightly tweak a couple of colours as I worked on the implementation of the colour palette to meet contrast requirements here and there and this was one of the colours I ended up having to modify 👍🏽 as long as we have this as a global variable and not an ad hoc colour in a stylesheet I think this is good.

@trallard
Copy link
Collaborator

I think you're referring to when the Pandas data table is put inside the example tabbed ipywidget.

Yes, that is what I was referring to. While this does not look as nice or polished as the other tables, reading the alternatives for aligning the style makes them seem more hacky/problematic than the current style mismatch, so we can leave them as they are for now (they are already much better than they were before, anyway).

Try to add our own styles to ipywidgets so they look good in dark mode. My guess is we could write a set of rules that work in most but not all cases. This would be a band-aid solution jupyter-widgets/ipywidgets#3675. If ipywidgets supported dark mode, we wouldn't have this dark-light-dark layering issue at all.

This would be perhaps the best solution. The linked issue does not have any follow-ups or responses, so perhaps we should discuss it internally and see if we can offer some time/capacity to make this move forward WDYT @gabalafou?

@trallard trallard merged commit a7c39b0 into pydata:main May 13, 2024
17 checks passed
@drammock drammock mentioned this pull request May 15, 2024
10 tasks
ivanov pushed a commit to ivanov/pydata-sphinx-theme that referenced this pull request Jun 5, 2024
* Implement @smeragoel's data table designs

* Working for nbsphinx (plus inside ipywidget)

* Add outer and column borders to data table

* Rename color vars

* Extend data-table styles to all tables

* Apply suggestions from code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: accessibility Issues related to accessibility issues or efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants