Skip to content

[Console] Fix floating tools rendering logic#54505

Merged
jloleysens merged 2 commits intoelastic:masterfrom
jloleysens:fix/console/floating-tools-rendering
Jan 13, 2020
Merged

[Console] Fix floating tools rendering logic#54505
jloleysens merged 2 commits intoelastic:masterfrom
jloleysens:fix/console/floating-tools-rendering

Conversation

@jloleysens
Copy link
Copy Markdown
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 t// release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Jan 10, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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.

// 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
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>
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 t// v7.6.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants