Refactor toolbar #396
Refactor toolbar #396
Conversation
|
|
|
There are some CSS issues still |
martinRenou
force-pushed
the
refactor_toolbar
branch
6 times, most recently
from
d980aa9
to
4c7ec7b
Compare
toolbar.mp4 |
|
@tacaswell @SylvainCorlay @ianhi if you have any thoughts :) Feedback would be super welcome |
|
The plot is now centered in the cell output |
martinRenou
force-pushed
the
refactor_toolbar
branch
2 times, most recently
from
0d1dd88
to
a86897c
Compare
Will look tonight or this weekend |
There was a problem hiding this comment.
Sorry for the delay @martinRenou
general refactor
I am 👍 on the code simplification
fade in/out
👍
I think the fade in and out is nice and my one suggestion is that it should be toggleable by a user.
plot centering
Currently 👎 but open to being convinced or improving this.
Was this necessary to get the fading to work?
The two main issues that the centering brought for me are:
-
Weird gap to the toolbar when the plot is small. This makes it inconvenient to use the toolbar because you have to move your mouse so far
-
The centering has a big impact on the dynamics of resizing the plot. It doubles the rate of resizing horizontally while leaving vertical resizing the same speed which feels very un-intuitive:
I think what happened is that this breaks the assumption that when were resizing a window the opposite edges (top and left) will stay in their positions. Maybe this could be reconfigured so that those edges stay fixed while resizing? Otherwise I think this ends up being more detrimental than beneficial (users can already center the plot using ipywidgets although that isn't super smooth).
|
Thanks for the review @ianhi :) I can revert the centering of the plot. This was not the initial purpose of the PR anyway. |
martinRenou
force-pushed
the
refactor_toolbar
branch
6 times, most recently
from
0e8d54d
to
376d61f
Compare
|
This pull request introduces 1 alert when merging 376d61f into 480a236 - view on LGTM.com new alerts:
|
martinRenou
force-pushed
the
refactor_toolbar
branch
2 times, most recently
from
3b431cd
to
fa6cec5
Compare
|
@ianhi Thanks again for your review. I took your comments into account:
|
martinRenou
force-pushed
the
refactor_toolbar
branch
2 times, most recently
from
f0d16f0
to
4629fe3
Compare
martinRenou
force-pushed
the
refactor_toolbar
branch
from
1fe95a9
to
afdebbf
Compare
| height: auto; | ||
| overflow: hidden; | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; |
There was a problem hiding this comment.
@martinRenou i thought the centering was reverted?
There was a problem hiding this comment.
It was.. This looks like a leftover. But this does not seem to affect the plot? It is not centered when I try latest.
There was a problem hiding this comment.
huh i was just seeing this behavior. let me try in a fresh env
There was a problem hiding this comment.
I see this on the binder link - maybe this is browser based. I'm on Firefox 93
There was a problem hiding this comment.
indeed on chrome it is not centered, but it is on firefox:
left = chrome
right = firefox
There was a problem hiding this comment.
I wish the visual regression tests caught that. We might be able to use playwright with Firefox as well. #422
There was a problem hiding this comment.
Thanks a lot for catching this!
Fix #299
toolbar.mp4