-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
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
Conversation
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. |
After some discussion with @GaelVaroquaux and @reshamas here are some possible alternatives: Arrows everywhereGray arrows everywhereOne arrowWhat does everything think? |
My favorit option is "Gray arrows everywhere" but the arrows becoming black when the corresponding title is hovered. |
@thomasjpfan I also like the gray arrows everywhere option. |
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). |
There was a problem hiding this 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)
There was a problem hiding this 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.
Here is a live version of the updated visualization:
|
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: "▶"; |
There was a problem hiding this comment.
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.
Here is a live version of the updated visualization.
|
There was a problem hiding this 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.
There was a problem hiding this 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
@scikit-learn/communication-team |
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:
I think after a user sees that one element can be clicked, they would start to hover around and click on the other elements.