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

feat: TableReport dark theme #1201

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

Conversation

rouk1
Copy link
Contributor

@rouk1 rouk1 commented Dec 13, 2024

This PR introduces a dark mode for the TableReport.
I tried to use as much as possible css variables all centralized in a _variables.css file.

UI preview

darkmode.mp4

@rouk1 rouk1 marked this pull request as ready for review December 13, 2024 16:34
@jeromedockes
Copy link
Member

Hi @rouk1 ! thanks so much for doing this 🚀 the TableReport looks really nice in the proposed dark mod🤩
also having more systematic naming for all css variables will definitely help with maintainability

I noticed that some aspects that are not related to colors are affected by the PR as well; here are a first few comments

  • I noticed standalone-report.css has been removed; this is where we set the font when we ask the report for a full html page (when we ask for a fragment it uses the font of the parent element). as a result the report now uses the default browser's font but I would like to keep a sans-serif font which I think tends to be a bit more compact and more readable for short text

  • that's really a detail but a line has appeared that separates the selected tab and its panel, I think it is a bit clearer that it is the selected tab when they are connected
    screenshot_2024-12-16T16:04:49+01:00

  • the selected column's card is not very well separated from the background anymore (I think due to removing the shadow)

screenshot_2024-12-16T16:05:05+01:00

  • the "Copied!" message from the copy to clipboard button used to be in reverse video, I think that helps notice it and separate it from the background

screenshot_2024-12-16T16:05:41+01:00

  • the copy buttons on the sample from the dataframe used to be bigger and in the top-left corner, now they are centered

screenshot_2024-12-17T10:27:15+01:00

  • also that sample text box used to have a max width and scroll horizontally which I think can be useful if the text is very long

  • in the summary statistics column the buttons are now smaller and have more padding which makes the icons hard to read IMO

screenshot_2024-12-16T16:07:24+01:00

In the summary statistics there used to be a thick gray line separating the first column when it overlaps the rest, I think it helps realize that this column is sticky and some content is hidden behind it due to horizontal scrolling, that has been removed

screenshot_2024-12-16T16:07:48+01:00

in the toggle tip shown below, there used to be a transition delay so that if we hover the ( i ) button and then move the mouse to hover over the text it doesn't disappear; now as soon as the mouse leaves the button the text disappears. also there used to be a ring around it when we click it

screenshot_2024-12-16T16:08:17+01:00

not sure if it is intentional but the background color of the header in the associations panel is gone, I don't have a strong opinion as to whether it is better or worse

screenshot_2024-12-16T16:14:30+01:00

most of these changes don't seem to be related to dark mode but it is true we had discussed that this is an opportunity to refactor the css a bit. so I'm not sure which of these are intentional or not, maybe it would help to have another quick call also with @glemaitre to discuss this? it might be easier interactively while looking at an open report.

now regarding the colors and the dark mode itself, here are a couple of comments:

  • the card in the first panel was green to show its association with the selected column, but that does not apply to the "distributions" panel so using a neutral gray color there was intentional
  • I think the orange in dark mode looks good but the reason we had used a different color is that we show histograms of numeric columns in blue and bar plots for categorical columns in orange so it might be good to use a third color for the card header to not give the impression there is a connection with the plot?
  • the axis lines and ticks in dark mode are not visible; I think we can find the previous pr one way to make sure they are

screenshot_2024-12-16T16:15:34+01:00

  • I think the white text on orange background does not have a very high contrast and is a bit hard to read

thanks again for all this work and LMK if you would like to schedule a call or I can also come to montparnasse sometime!

@rouk1
Copy link
Contributor Author

rouk1 commented Dec 17, 2024

Thanks for the review @jeromedockes, I'll try to work on your feedbacks on friday.

@jeromedockes
Copy link
Member

cool, thanks!!

@rouk1
Copy link
Contributor Author

rouk1 commented Dec 20, 2024

Hello @jeromedockes !

I think I've addressed your feedbacks. Please let me know if you want something else or if I miss something.

tackled feedbacks

  • standalone-report.css has been re-added (using new css vars nomenclature)
  • selected tab border bottom color is now equal to background color
  • fix column summaries shadow
  • copy button tooltip is now inverted and bold
  • copy text box as a max width of 100% hence it will never be larger than it's container (a horizontal scroller will appear at need)
  • summary statistics is now bigger and icons fits it
  • grey line that indicates that first two columns are sticky is visible to me
  • toggled tip content is now part oh the hover, hence it does not disappear when hovering it
  • tip button outline is now blue
  • association table header now uses default tertiary background color
  • card header are now in highlight color only whena ssociated with a table column
  • primary highlight color is now raisin instead of orange, it provides high contrast and is not related to plot colors
  • use 4 spaces in CSS to avoid large diff

the axis lines and ticks in dark mode are not visible; I think we can find the previous pr one way to make sure they are

Not sure on how I may address this.

UI preview

sk-tr-dm-pr.mp4

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