Stack cards horizontally in distribution tab by GaelVaroquaux · Pull Request #1259 · skrub-data/skrub

GaelVaroquaux · 2025-03-19T17:56:02Z

To avoid vertical scrolling in IDEs.

Related to #1257

To avoid vertical scrolling in IDEs

jeromedockes

thanks a lot @GaelVaroquaux ! I think this can be nice in notebooks and vs code.

the downside as you mentioned in the issue it does reduce the number of cards we can see at once and force us to scroll horizontally to see them.

when in a full page I think it makes a suboptimal use of space:

here I would rather not have to scroll

jeromedockes · 2025-03-19T18:13:09Z

    cursor: pointer;
 }

+.float-left {


why do we float the bar & buttons? it results in wasting space and looks a bit weird IMO

also moving the buttons to the right means there is much less space for the tooltip

which was one reason for having it on the left

Vertical space.

This is the user experience, even with the button on the side

The table report takes a full screen. I cannot see the line that I have run to generate it. It's really annoying. It's a major usability problem, and TBH a reason for me not wanting to use it.

We can improve the tooltip, but it really does not matter compared the waste of vertical space

Also, I think that the problem that you flagged cannot happen with the new code that I have committed.

GaelVaroquaux · 2025-03-19T21:22:38Z

This was meant to be a quick PR so that people told me if the idea was a good one.

This tool is going to be used mostly in IDEs and notebook, so the vertical scaling is crucial (I'm not the only saying this). This is why I moved the button to the side. I'll do it in a cleaner way than the float :).

Yes, we can special case the "open", but it makes the code more complicated and I wonder if we are wasting time on a feature that is almost not going to be used.

rcap107 · 2025-03-20T09:13:52Z

I really like this feature, it's far more convenient to use in VSCode now.

I don't have issues with vertical space either, I had to zoom in a lot to get to the point where the line of code was cut, so that's not a problem on my side.

One thing I noticed is that I was working on the code by looking at the TableReport, then writing something and executing the cell, then going back to the TR to see what else needed doing. That mean I was scrolling up all the time to see the columns and the TR tabs, and this feature addresses the endless scrolling.

jeromedockes

LGTM once the suggestion about relaxing the flex container's width is added

jeromedockes · 2025-03-20T09:11:18Z

 .report-bottom-border {
    width: calc(100% - 2 * var(--radius));
    border-bottom: var(--border-s) solid var(--color-border-secondary);
-    margin: var(--spacing-m) auto 0 auto;


we can replace --spacing-m with --spacing-s to make it smaller but having the bottom border touch the content looks a bit weird:

otherwise we can also just remove that border

OK, it's that we need to move the "margin" spacing from this container to a "padding" spacing of the enclosed object, else the drop shadow of the cards looks strange here

to prevent cropping the shadows isn't it easier to keep the margin that used to be around the cards? (it is a bit large but we can reduce it & make it much smaller on top and left)

I see this padding needed only the statistics and associations panel. I've added it

jeromedockes · 2025-03-20T09:17:19Z

-    margin: 0;
+/* A bit of space between the bar and the buttons */
+.column-selection-buttons {
+    padding-left: 10pt;


Suggested change

padding-left: 10pt;

padding-left: var(--spacing-m);

jeromedockes · 2025-03-20T09:32:29Z

for the buttons, indeed there is not the issue I pointed out anymore. one drawback of having them on the side is that they move when we click them.

we could also consider just removing those buttons and the bar because I don't know how many people would use them (either because they don't need it or haven't figured out what it does)

GaelVaroquaux · 2025-03-20T10:52:41Z

Could we have the buttons and the bar appear only when the selection is not empty? Ie when clicking on the tick box of the cards?

…

On Mar 20, 2025, 10:37, at 10:37, "Jérôme Dockès" ***@***.***> wrote: jeromedockes left a comment (skrub-data/skrub#1259) for the buttons, indeed there is not the issue I pointed out anymore. one drawback of having them on the side is that they move when we click them. we could also consider just removing those buttons and the bar because I don't know how many people would use them (either because they don't need it or haven't figured out what it does) -- Reply to this email directly or view it on GitHub: #1259 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

GaelVaroquaux · 2025-03-20T12:12:58Z

For the button:

we could also consider just removing those buttons and the bar because I don't know how many people would use them (either because they don't need it or haven't figured out what it does)

I think that what would be really cool would be to have the button and the bar visible only when the bar is not empty. Ie, clicking on the tickbox would have them show up. Do you know easily how to add this, @jeromedockes ?

Co-authored-by: Jérôme Dockès <jerome@dockes.org>

jeromedockes · 2025-03-20T12:23:51Z

I think that what would be really cool would be to have the button and the bar visible only when the bar is not empty. Ie, clicking on the tickbox would have them show up. Do you know easily how to add this, @jeromedockes ?

yes that's easy to do but the card (and tickbox we are clicking) will jump when we click on the first tickbox (or uncheck the last), and selecting all columns to then remove some becomes check one to make the button appear, then select all. so all in all I'm not sure it's better than the solution in this PR. I suggest we keep as it is in this PR and experiment with that in a separate one

GaelVaroquaux · 2025-03-20T12:26:57Z

I suggest we keep as it is in this PR and experiment with that in a separate one

I agree! :)

I do like the idea of making this not visible by default. It would also decrease the visual crowding, which would be great

GaelVaroquaux · 2025-03-20T12:42:41Z

OK, I think that this is ready for merge

GaelVaroquaux · 2025-03-20T12:46:48Z

to prevent cropping the shadows isn't it easier to keep the margin that used to be around the cards?

That does not work, because it applies to the enclosing element and thus the shadow stop inside the enclosed element, leaving the white margin outside.

jeromedockes · 2025-03-20T13:26:48Z

That does not work, because it applies to the enclosing element and thus the shadow stop inside the enclosed element, leaving the white margin outside.

I'm not sure I understood 100% what you mean but I still see the shadows being cropped with the current pr's version:

we can avoid it either by having a margin around the cards

diff --git a/skrub/_reporting/_data/templates/column-summaries.css b/skrub/_reporting/_data/templates/column-summaries.css
index a037513a..bbcbc341 100644
--- a/skrub/_reporting/_data/templates/column-summaries.css
+++ b/skrub/_reporting/_data/templates/column-summaries.css
@@ -9,7 +9,7 @@
     display: flex;
     flex-direction: row;
     flex-wrap: nowrap;
-    gap: var(--spacing-l);
+    gap: 0;
     width: max-content;
 }
 
@@ -25,9 +25,14 @@
 }
 
 .column-summary-group > .card {
-    margin: 0;
+    margin: var(--spacing-s) var(--spacing-l) var(--spacing-l) var(--spacing-l);
 }
 
+.column-summary-group > .card + .card {
+    margin-left: 0;
+}
+
+
 /* Individual column summary cards */
 /* ------------------------------- */

or padding on the parent

diff --git a/skrub/_reporting/_data/templates/column-summaries.css b/skrub/_reporting/_data/templates/column-summaries.css
index a037513a..7aac0e7d 100644
--- a/skrub/_reporting/_data/templates/column-summaries.css
+++ b/skrub/_reporting/_data/templates/column-summaries.css
@@ -11,6 +11,7 @@
     flex-wrap: nowrap;
     gap: var(--spacing-l);
     width: max-content;
+    padding: var(--spacing-s) var(--spacing-l) var(--spacing-l) var(--spacing-l);
 }
 
 /* Increase the gap between buttons & columns to make it clearer the buttons are

GaelVaroquaux · 2025-03-20T13:35:26Z

I'm fine with your proposed solution as long as it doesn't add too much vertical whitespace. Do you want to send a PR here? I'm totally out of time on this, I did this PR as a quick demo of a solution to the problem, and I'm spending why more time than I can afford on this

jeromedockes · 2025-03-21T08:43:52Z

just so we have the full discussion here: there are also some comments in

GaelVaroquaux#3

jeromedockes · 2025-03-21T08:50:45Z

not a big deal but in the first tab also, removing the bottom padding means the column summary card overlaps the border

GaelVaroquaux · 2025-03-21T09:33:13Z

not a big deal but in the first tab also, removing the bottom padding means the column summary card overlaps the border

Fun thing: I'm not getting this, it must be a firefox vs chrome difference. Hence the importance of testing on many browsers

And color the cards

GaelVaroquaux · 2025-03-21T13:45:17Z

Card coloring implemented.

This should now be good for review

jeromedockes · 2025-03-21T14:52:33Z

I think we are losing some contrast (unless we also change the text), https://webaim.org/resources/contrastchecker/ complains about these (there may be more):

the bar plots & background are also quite close

and this is a detail but buttons don't match the background anymore, not sure if that's good or bad

the background color makes it harder to have a good contrast and I'm not sure the benefit it brings makes up for that.

maybe we need to tone them down or change the text color. to avoid delaying the horizontal scroll thing we can also merge it like this and revisit the colors in another pr

GaelVaroquaux · 2025-03-21T14:58:06Z

Ok, can you take over.
I really must be working on the grant proposal

jeromedockes · 2025-03-27T11:49:59Z

ok the contrast between the text and background should be ok now, @GaelVaroquaux lmk if it's ok for you

jeromedockes · 2025-03-27T11:51:35Z

I don't think the changes to the bottom border are a big improvement, now it is not centered and it touches the scrollbar. but it's kind of a detail

now:

main branch:

(we could reduce the margin without removing it entirely, and also keep the centering)

jeromedockes · 2025-03-27T11:52:00Z

(closed by mistake)

GaelVaroquaux · 2025-03-28T10:45:11Z

I don't think the changes to the bottom border are a big improvement,

Agreed

GaelVaroquaux · 2025-03-28T10:47:14Z

 .columns-in-sample-tab .card-header {
    position: relative;
-    background-color: var(--color-background-highlight-primary);
-    border-block-end: var(--border-s) solid var(--color-border-highlight);


@jeromedockes : do you think that this is the faulty change, regarding the border?

no I think it is this one

if it's ok for you I'll restore the deleted rule it but making the vertical margin much smaller

Yes, but much smaller :).

Thanks!

jeromedockes · 2025-03-28T14:23:40Z

    /* shadows */
    --color-shadow: hsl(0, 0%, 63%);
-    --color-shadow-keyboard: rgb(255, 255, 255, 0.5);
+    --color-shadow-keyboard: var(--color-border-secondary);


that one is not related to this pr but I noticed it and figured let's fix it here and not do another pr for 2 lines of diff

jeromedockes

should be ok now!

GaelVaroquaux · 2025-03-28T15:32:50Z

@rcap107 : can you check the visuals (eg the rendered docs) and merge if happy?

Co-authored-by: Jérôme Dockès <jerome@dockes.org>

Stack cards horizontally in distribution tab

cbb4c29

To avoid vertical scrolling in IDEs

GaelVaroquaux added the TableReport anything related to the TableReport label Mar 19, 2025

Changelog

717c2f7

GaelVaroquaux mentioned this pull request Mar 19, 2025

Some issues with TableReport I noticed while working with many columns #1257

Closed

jeromedockes reviewed Mar 19, 2025

View reviewed changes

Cleaner CSS

e1db710

jeromedockes reviewed Mar 20, 2025

View reviewed changes

GaelVaroquaux and others added 2 commits March 20, 2025 13:13

Update skrub/_reporting/_data/templates/column-summaries.css

04d79d0

Co-authored-by: Jérôme Dockès <jerome@dockes.org>

Add padding

1a73198

Drop the drop-shadow

0b10488

And color the cards

increase contrast

6b4d79e

jeromedockes closed this Mar 27, 2025

jeromedockes reopened this Mar 27, 2025

GaelVaroquaux commented Mar 28, 2025

View reviewed changes

jeromedockes added 2 commits March 28, 2025 15:09

padding & margins

3a6cb24

fix keyboard shortcuts borders

11ba054

jeromedockes reviewed Mar 28, 2025

View reviewed changes

jeromedockes approved these changes Mar 28, 2025

View reviewed changes

jeromedockes and others added 2 commits March 28, 2025 15:34

_

97bcc08

Tiny color fix

a3b6328

make green text darker

788f7f5

rcap107 merged commit 82c3251 into skrub-data:main Mar 28, 2025

rcap107 pushed a commit to rcap107/skrub that referenced this pull request Apr 2, 2025

Stack cards horizontally in distribution tab (skrub-data#1259)

6c5b1de

Co-authored-by: Jérôme Dockès <jerome@dockes.org>

rcap107 pushed a commit that referenced this pull request Apr 2, 2025

Stack cards horizontally in distribution tab (#1259)

6756acf

Co-authored-by: Jérôme Dockès <jerome@dockes.org>

Uh oh!

Conversation

GaelVaroquaux commented Mar 19, 2025

Uh oh!

jeromedockes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GaelVaroquaux commented Mar 19, 2025

Uh oh!

rcap107 commented Mar 20, 2025

Uh oh!

jeromedockes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jeromedockes commented Mar 20, 2025

Uh oh!

GaelVaroquaux commented Mar 20, 2025 via email

Uh oh!

GaelVaroquaux commented Mar 20, 2025

Uh oh!

jeromedockes commented Mar 20, 2025

Uh oh!

GaelVaroquaux commented Mar 20, 2025

Uh oh!

GaelVaroquaux commented Mar 20, 2025

Uh oh!

GaelVaroquaux commented Mar 20, 2025 via email

Uh oh!

jeromedockes commented Mar 20, 2025

Uh oh!

GaelVaroquaux commented Mar 20, 2025

Uh oh!

jeromedockes commented Mar 21, 2025

Uh oh!

jeromedockes commented Mar 21, 2025

Uh oh!

GaelVaroquaux commented Mar 21, 2025

Uh oh!

GaelVaroquaux commented Mar 21, 2025

Uh oh!

jeromedockes commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GaelVaroquaux commented Mar 21, 2025

Uh oh!

jeromedockes commented Mar 27, 2025

Uh oh!

jeromedockes commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeromedockes commented Mar 27, 2025

Uh oh!

GaelVaroquaux commented Mar 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux Mar 19, 2025 •

edited

Loading

jeromedockes commented Mar 21, 2025 •

edited

Loading

jeromedockes commented Mar 27, 2025 •

edited

Loading