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

a test case to illustrate issue #14871 #15259

Closed
wants to merge 4 commits into from

Conversation

parmentelat
Copy link
Contributor

References

issue #14871

Code changes

one more test case, with related input and outputs

notes

User-facing changes

none

Backwards-incompatible changes

none

@jupyterlab-probot
Copy link

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

@parmentelat
Copy link
Contributor Author

also I had to run git commit --no-verify, the prettier stage was failing, even though jlpm run prettier seemed happy
please let me know how to deal with that

@krassowski
Copy link
Member

krassowski commented Oct 11, 2023

jlpm run prettier is only prettier, there is also eslint which might have caught that getSelectionRange is unused and needs to be used or removed. In general jlpm run lint from root of the repo should work (it includes both prettier and eslint).

@parmentelat
Copy link
Contributor Author

parmentelat commented Oct 11, 2023

I must have misunderstood, I had the impression it was the prettier stage that was wrong..
anyway, I have removed the unused function and it is now all good wrt committing
thanks @krassowski

@krassowski krassowski added the bug label Oct 11, 2023
@krassowski krassowski added this to the 4.0.x milestone Oct 11, 2023
@parmentelat
Copy link
Contributor Author

trying to improve the situation, here's a few thoughts to share

level1

the first-level bug has to do with the search dialog having lasting effects
and indeed, it will be an obvious improvement
if giving up on the search dialog (using either Escape or the cross icon)
brings us back in a state where nothing is being searched

level2

however, playing with the search a little more, I came to realize that,
when the search dialog is still on, it feels like the job in charge of outlining stuff
sometimes has a very intrusive behaviour, notably putting some cells in edit mode while they were not,
which is imho wrong in itself, and worse still it often leads to arbitrary cells becoming in focus (in the viewport)
as a result this intrusion is very often devastatingly confusing for the user,
because it makes one lose one's train of thought by scrolling up or down with no apparent sense

my own experience has been to face level2, so I tried to get rid off the issue by turning off the search dialog,
but it was not enough because of level1

So I guess what I'm saying is, I might eventually be able to fix stage1 as part of this PR, I'll try at least
but even so, there will remain some agreement to reach on how the search outlining business
is expected to behave, and imho the expected behaviour would be:

  • no change of editing mode
  • no scrolling whatsoever

but I guess that's just me
and apologies for getting a little off-track as compared to the initial target of this PR

@krassowski
Copy link
Member

krassowski commented Dec 10, 2023

Thanks again for your work here @parmentelat 🎉, closing this in favour of #15443 which includes your commits from this PR. Please feel welcome to review the new PR :).

@krassowski krassowski closed this Dec 10, 2023
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.

None yet

2 participants