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

[Console] Fix floating tools rendering logic #54505

Merged

Conversation

jloleysens
Copy link
Contributor

Summary

After #52270 was merged, a regression snuck in that made the floating tools rendering on screen width changes break.

Additionally I managed to get an infinite loop going fairly consistently at one point by resizing this text (which I have been struggling to repro):

GET _search
{
  "query": {
    "match_all": """"test"""
  }
}

GET _cat/indices

POST test_1
{}

POST _sqasddasdasdsddddddddddddddddddddddddd
{
  "query": """
  SELECT * FROM "TABLE"
  """
}

How to review

Try and break the resizing behaviour using the resizer in Console. Specifically dragging to the left and squishing any text that is there. Look out for any weird behaviour.

@jloleysens jloleysens added Feature:Console Dev Tools Console Feature v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Jan 10, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally and did not spot any issues.

@@ -297,30 +297,30 @@ export class LegacyCoreEditor implements CoreEditor {
// pageY is relative to page, so subtract the offset
// from pageY to get the new top value
const offsetFromPage = $(this.editor.container).offset()!.top;
const startRow = range.start.lineNumber - 1;
const startLine = range.start.lineNumber;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: at first glance, i don't think the const name startLine is clear in how it differs from firstLine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After this #52270 was merged a distinction was created between row (zero indexed) and line (not-zero indexed). This was informed by the approach taken by Monaco 😅

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jloleysens jloleysens merged commit ccd36b3 into elastic:master Jan 13, 2020
@jloleysens jloleysens deleted the fix/console/floating-tools-rendering branch January 13, 2020 10:44
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 13, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 13, 2020
…age-offset-floating-tooltip

* 'master' of github.com:elastic/kibana:
  [Maps] refactor isPointsOnly, isLinesOnly, and isPolygonsOnly to make synchronous (elastic#54067)
  Fix icon path in tutorial introduction (elastic#49684)
  [State Management] State containers improvements (elastic#54436)
  Fix floating tools rendering logic (elastic#54505)
  Handle another double quote special case (elastic#54474)
  [Home][Tutorial] Add data UI for IBM MQ Filebeat module (elastic#54238)
  fix(package): upgrade transitive dependency elliptic to v6.5.2 (elastic#54476)
  [Graph] Fix various a11y issues (elastic#54097)

# Conflicts:
#	src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/legacy_core_editor.ts
jloleysens added a commit that referenced this pull request Jan 13, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Jan 13, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Console Dev Tools Console Feature release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants