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

[ES|QL] Fix editor cursor jumpiness #202389

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

davismcphee
Copy link
Contributor

@davismcphee davismcphee commented Dec 1, 2024

Summary

Closes #191216

This PR fixes my least favourite editor bug - the cursor jumpiness! It turns out this was just due to Monaco editor being picky about styles. I removed the margin from the editor since that's what was causing the cursor to be off, and then added an equivalent number of pixels to lineDecorationsWidth so it's visually the same.

I tracked the issue back to this PR in 8.15: #186345. It may not make it in the last planned 8.15 patch release, but we can backport there anyway just in case.

Before:

before.mp4

After:

after.mp4

Fixes #191216.

Checklist

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

@davismcphee davismcphee added release_note:fix backport:prev-major Backport to (8.x, 8.17, 8.16) the previous major branch and other branches in development Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana labels Dec 1, 2024
@davismcphee davismcphee self-assigned this Dec 1, 2024
@davismcphee davismcphee marked this pull request as ready for review December 2, 2024 01:00
@davismcphee davismcphee requested review from a team as code owners December 2, 2024 01:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-esql (Team:ESQL)

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Thanx a ton for the contribution Davis! 🙌
This was very annoying indeed but works great with your changes!

The only thing that I noticed is that for some reason in this PR the scrollbar looks differently

image

Here how it looks in main
image

@davismcphee
Copy link
Contributor Author

@stratoula Turns out the smaller scrollbars were due to part of the editor being rendered off screen from the old margins. The default scrollbars are thick though and we can get the same thin look by updating the actual scrollbar widths. Updated here: 4a99d43.

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

This looks awesome, LGTM. Thanx a lot for the contribution 🙌

@stratoula stratoula added v9.0.0 v8.18.0 backport:version Backport to applied version labels and removed backport:prev-major Backport to (8.x, 8.17, 8.16) the previous major branch and other branches in development labels Dec 3, 2024
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
esql 196.6KB 196.5KB -130.0B

History

cc @davismcphee

@davismcphee davismcphee merged commit 22141cb into elastic:main Dec 4, 2024
8 checks passed
@davismcphee davismcphee deleted the fix-esql-cursor branch December 4, 2024 00:12
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12150400643

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 4, 2024
## Summary

Closes elastic#191216

This PR fixes my least favourite editor bug - the cursor jumpiness! It
turns out this was just due to Monaco editor being picky about styles. I
removed the margin from the editor since that's what was causing the
cursor to be off, and then added an equivalent number of pixels to
`lineDecorationsWidth` so it's visually the same.

I tracked the issue back to this PR in 8.15: elastic#186345. It may not make it
in the last planned 8.15 patch release, but we can backport there anyway
just in case.

Before:

https://github.com/user-attachments/assets/924bbdb4-0ba6-446c-a4ad-63332f43f158

After:

https://github.com/user-attachments/assets/864bedd0-c711-4910-b7a7-6a54e27e9148

Fixes elastic#191216.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

(cherry picked from commit 22141cb)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Dec 4, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] Fix editor cursor jumpiness
(#202389)](#202389)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Davis
McPhee","email":"davis.mcphee@elastic.co"},"sourceCommit":{"committedDate":"2024-12-04T00:12:04Z","message":"[ES|QL]
Fix editor cursor jumpiness (#202389)\n\n## Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/191216\r\n\r\nThis PR fixes my
least favourite editor bug - the cursor jumpiness! It\r\nturns out this
was just due to Monaco editor being picky about styles. I\r\nremoved the
margin from the editor since that's what was causing the\r\ncursor to be
off, and then added an equivalent number of pixels
to\r\n`lineDecorationsWidth` so it's visually the same.\r\n\r\nI tracked
the issue back to this PR in 8.15: #186345. It may not make it\r\nin the
last planned 8.15 patch release, but we can backport there
anyway\r\njust in
case.\r\n\r\nBefore:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/924bbdb4-0ba6-446c-a4ad-63332f43f158\r\n\r\nAfter:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/864bedd0-c711-4910-b7a7-6a54e27e9148\r\n\r\nFixes
#191216.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [ ] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] If a plugin
configuration key changed, check if it needs to be\r\nallowlisted in the
cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This was checked for breaking HTTP API changes, and any
breaking\r\nchanges have been approved by the breaking-change committee.
The\r\n`release_note:breaking` label should be applied in these
situations.\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] The PR description includes
the appropriate Release Notes section,\r\nand the correct
`release_note:*` label is applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"22141cbbfa1c7c00800fe340032f84ee87ba0222","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","v9.0.0","Feature:ES|QL","Team:ESQL","backport:version","v8.18.0"],"title":"[ES|QL]
Fix editor cursor
jumpiness","number":202389,"url":"https://github.com/elastic/kibana/pull/202389","mergeCommit":{"message":"[ES|QL]
Fix editor cursor jumpiness (#202389)\n\n## Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/191216\r\n\r\nThis PR fixes my
least favourite editor bug - the cursor jumpiness! It\r\nturns out this
was just due to Monaco editor being picky about styles. I\r\nremoved the
margin from the editor since that's what was causing the\r\ncursor to be
off, and then added an equivalent number of pixels
to\r\n`lineDecorationsWidth` so it's visually the same.\r\n\r\nI tracked
the issue back to this PR in 8.15: #186345. It may not make it\r\nin the
last planned 8.15 patch release, but we can backport there
anyway\r\njust in
case.\r\n\r\nBefore:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/924bbdb4-0ba6-446c-a4ad-63332f43f158\r\n\r\nAfter:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/864bedd0-c711-4910-b7a7-6a54e27e9148\r\n\r\nFixes
#191216.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [ ] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] If a plugin
configuration key changed, check if it needs to be\r\nallowlisted in the
cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This was checked for breaking HTTP API changes, and any
breaking\r\nchanges have been approved by the breaking-change committee.
The\r\n`release_note:breaking` label should be applied in these
situations.\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] The PR description includes
the appropriate Release Notes section,\r\nand the correct
`release_note:*` label is applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"22141cbbfa1c7c00800fe340032f84ee87ba0222"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202389","number":202389,"mergeCommit":{"message":"[ES|QL]
Fix editor cursor jumpiness (#202389)\n\n## Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/191216\r\n\r\nThis PR fixes my
least favourite editor bug - the cursor jumpiness! It\r\nturns out this
was just due to Monaco editor being picky about styles. I\r\nremoved the
margin from the editor since that's what was causing the\r\ncursor to be
off, and then added an equivalent number of pixels
to\r\n`lineDecorationsWidth` so it's visually the same.\r\n\r\nI tracked
the issue back to this PR in 8.15: #186345. It may not make it\r\nin the
last planned 8.15 patch release, but we can backport there
anyway\r\njust in
case.\r\n\r\nBefore:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/924bbdb4-0ba6-446c-a4ad-63332f43f158\r\n\r\nAfter:\r\n\r\n\r\nhttps://github.com/user-attachments/assets/864bedd0-c711-4910-b7a7-6a54e27e9148\r\n\r\nFixes
#191216.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [ ] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] If a plugin
configuration key changed, check if it needs to be\r\nallowlisted in the
cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This was checked for breaking HTTP API changes, and any
breaking\r\nchanges have been approved by the breaking-change committee.
The\r\n`release_note:breaking` label should be applied in these
situations.\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] The PR description includes
the appropriate Release Notes section,\r\nand the correct
`release_note:*` label is applied per
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"22141cbbfa1c7c00800fe340032f84ee87ba0222"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Davis McPhee <davis.mcphee@elastic.co>
hop-dev pushed a commit to hop-dev/kibana that referenced this pull request Dec 5, 2024
## Summary

Closes elastic#191216

This PR fixes my least favourite editor bug - the cursor jumpiness! It
turns out this was just due to Monaco editor being picky about styles. I
removed the margin from the editor since that's what was causing the
cursor to be off, and then added an equivalent number of pixels to
`lineDecorationsWidth` so it's visually the same.

I tracked the issue back to this PR in 8.15: elastic#186345. It may not make it
in the last planned 8.15 patch release, but we can backport there anyway
just in case.

Before:


https://github.com/user-attachments/assets/924bbdb4-0ba6-446c-a4ad-63332f43f158

After:


https://github.com/user-attachments/assets/864bedd0-c711-4910-b7a7-6a54e27e9148

Fixes elastic#191216.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 9, 2024
## Summary

Closes elastic#191216

This PR fixes my least favourite editor bug - the cursor jumpiness! It
turns out this was just due to Monaco editor being picky about styles. I
removed the margin from the editor since that's what was causing the
cursor to be off, and then added an equivalent number of pixels to
`lineDecorationsWidth` so it's visually the same.

I tracked the issue back to this PR in 8.15: elastic#186345. It may not make it
in the last planned 8.15 patch release, but we can backport there anyway
just in case.

Before:


https://github.com/user-attachments/assets/924bbdb4-0ba6-446c-a4ad-63332f43f158

After:


https://github.com/user-attachments/assets/864bedd0-c711-4910-b7a7-6a54e27e9148

Fixes elastic#191216.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
## Summary

Closes elastic#191216

This PR fixes my least favourite editor bug - the cursor jumpiness! It
turns out this was just due to Monaco editor being picky about styles. I
removed the margin from the editor since that's what was causing the
cursor to be off, and then added an equivalent number of pixels to
`lineDecorationsWidth` so it's visually the same.

I tracked the issue back to this PR in 8.15: elastic#186345. It may not make it
in the last planned 8.15 patch release, but we can backport there anyway
just in case.

Before:


https://github.com/user-attachments/assets/924bbdb4-0ba6-446c-a4ad-63332f43f158

After:


https://github.com/user-attachments/assets/864bedd0-c711-4910-b7a7-6a54e27e9148

Fixes elastic#191216.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels Feature:ES|QL ES|QL related features in Kibana release_note:fix Team:ESQL ES|QL related features in Kibana v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ES|QL] Jumpiness when selecting part of the ES|QL query in the editor
5 participants