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

Wait for the cell toolbar items to be rendered the first time before looking for overlap #16291

Merged
merged 3 commits into from
May 9, 2024

Conversation

brichet
Copy link
Contributor

@brichet brichet commented May 6, 2024

When the cell toolbar is attached the first time, the items may not be rendered when we try to see if it overlap the cell content.
This make the toolbar to be displayed when it should not (see #15305 (comment))

Peek 2024-05-06 16-36

References

Related to #15305, and more specifically should fix #15305 (comment)

Code changes

  • hide the toolbar by default when it is added to the cell, to avoid flickering
  • wait for all the items to be rendered, using requestIdleCallback() instead of requestAnimationFrame() before looking for overlap (this should wait for changes in the cell and in the toolbar to be effective).

User-facing changes

Avoid the toolbar to be displayed when it should not.

Backwards-incompatible changes

None

Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@brichet brichet marked this pull request as ready for review May 7, 2024 06:15
@krassowski krassowski added this to the 4.2.x milestone May 7, 2024
Copy link
Contributor

@JasonWeill JasonWeill left a comment

Choose a reason for hiding this comment

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

I see a slight (~1 frame) period after focusing a cell before the toolbar appears, but this should be OK in day-to-day use. Good work!

@krassowski
Copy link
Member

I see a slight (~1 frame) period after focusing a cell before the toolbar appears

The delay is slightly distracting annoying when clicking on the editor. That said, when just adding cells, I think the delay is less annoying than the previous behaviour of rendering one button at a time (captured with x6 CPU throttling):

Before After
before after

@krassowski
Copy link
Member

krassowski commented May 9, 2024

Maybe there should be an opacity-based transition so that they appear smoothly?

Edit: I opened #16310 to discuss

@krassowski krassowski merged commit 3c13610 into jupyterlab:main May 9, 2024
81 checks passed
@brichet brichet deleted the cell-toolbar_logic branch May 13, 2024 12:26
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.

Should we improve cell toolbar logic or disable cell toolbar in tests?
3 participants