Skip to content

ENH Adds arrow on top level estimator in html repr #21298

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

Merged
merged 15 commits into from
Nov 29, 2021

Conversation

thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Oct 10, 2021

Reference Issues/PRs

Closes #21288

What does this implement/fix? Explain your changes.

This PR adds an arrow to the top most estimator, that changes when it is clicked:

arrow

I think after a user sees that one element can be clicked, they would start to hover around and click on the other elements.

@adrinjalali
Copy link
Member

I love this, how hard would it be to add the arrow to all of them? Cause now it seems like the user needs to click the top most button to enable expansion of the subestimators, I think.

@thomasjpfan
Copy link
Member Author

After some discussion with @GaelVaroquaux and @reshamas here are some possible alternatives:

Arrows everywhere

arrow_everywhere

Gray arrows everywhere

gray-arrow

One arrow

one-arrow

What does everything think?

@GaelVaroquaux
Copy link
Member

My favorit option is "Gray arrows everywhere" but the arrows becoming black when the corresponding title is hovered.

@reshamas
Copy link
Member

@thomasjpfan I also like the gray arrows everywhere option.

@jnothman
Copy link
Member

I think this change is good, but there's something aesthetically awkward about the sizing or spacing of the grey arrows, perhaps because of the monospace font after them. Maybe the arrow needs to be smaller?

What do these arrows do?
Uploading image.png…

@GaelVaroquaux
Copy link
Member

I think that this an improvement over what we have, and I am in favor of merging as such (in particular given that we now have conflicts, and the more we wait, the worse it is going to get).

Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

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

LGTM (once the conflicts are fixed)

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM after fixing conflicts.

Thank you for this notable usability improvement, @thomasjpfan.

@jnothman
Copy link
Member

Trying to get specific re my nitpick about the shape/alignment of the triangles. Triangles with this function are usually thinner, and are sometimes not closed:
image
image

As seen here, the vertical alignment appears to be midway between the baseline and the top of the text, while your triangles sit far lower.

I'm not sure how to achieve this with CSS.

@thomasjpfan
Copy link
Member Author

Here is a live version of the updated visualization:

  1. On hover, the arrow turns black.
  2. The arrow itself is smaller.
  3. Arrows everywhere.

@GaelVaroquaux
Copy link
Member

Nitpick: when a panel is expended, the down arrow become darker on hover of the full panel, but only the top line collapses the panel. It would be if the array became darker only when hovering on elements for which a click will induce a action.

@@ -179,6 +180,19 @@ def _write_estimator_html(
box-sizing: border-box;
text-align: center;
}
#$id label.sk-toggleable__label-arrow:before {
content: "▶";
Copy link
Member

Choose a reason for hiding this comment

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

have you tried ▸/▾ instead of ▶︎/▼? They are less imposing.

@thomasjpfan
Copy link
Member Author

Here is a live version of the updated visualization.

  1. Hover state fixed as suggested by ENH Adds arrow on top level estimator in html repr #21298 (comment)
  2. Used ▸/▾ instead of ▶︎/▼.

Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

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

LGTM, +1 for merge.

The code is simpler than the previous version, which is great.

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

+1 for merge after fixing the CI

@jjerphan jjerphan merged commit 299364c into scikit-learn:main Nov 29, 2021
@reshamas
Copy link
Member

@scikit-learn/communication-team
FYI: for sharing on social media

samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UX of the HTML repr of estimators
6 participants