[ES|QL] Enhances the inline documentation experience #192156
Conversation
stratoula
changed the title
| @@ -89,15 +88,15 @@ ES|QL can access the following metadata fields: | |||
| Use the \`METADATA\` directive to enable metadata fields: | |||
|
|
|||
| \`\`\` | |||
| FROM index [METADATA _index, _id] | |||
There was a problem hiding this comment.
ℹ️ Irrelevant with the change but I decided to fix the docs for metadata in this PR too
| @@ -14,7 +14,6 @@ In order to enable text based languages on your unified search bar add `textBase | |||
|
|
|||
|
|
|||
| ## Languages supported | |||
| - SQL: based on the Elasticsearch sql api | |||
There was a problem hiding this comment.
ℹ️ Moved to the language package
|
@ryankeairns thanx a lot, with your changes looks even better :) I merged your PR, I added some i18n labels that were missing and also did this:
I am also thinking to follow up with this stratoula#33. Wdty? |
|
The FT failure is irrelevant with this PR. |
Yep, makes good sense! |
|
/ci |
There was a problem hiding this comment.
We are three days short of the 1-year anniversary for the issue that this PR closes. In the spirit of making much needed progress over perfection (the docs stay open now!), I say we get this merged and plan to follow up with a few items + address any new feedback.
Possible follow-on items for improving the inline docs experience include:
- Esql docs flyout expandable2 stratoula/kibana#33
- Closing other flyouts when opening this one and vice versa
- (not specific to this set of work) Highlight found text upon using the search input
- Layout adjustments for the inline editor (Marco's feedback)
|
Found an issue :/
The steps to reproduce this are:
Result 
|
There was a problem hiding this comment.
No objections on the technical side!
The docs generation script wasn't quite working so I fixed that in fe970f4. I also took the liberty of moving it back to a root-level scripts directory since it isn't really part of the source code of the package, and that matches the pattern I laid in kbn-esql-validation-autocomplete. That said, lmk if you disagree.
I do think the spacing is a little weird here and there, for example these lists:
packages/kbn-language-documentation-popover/src/components/as_flyout/index.test.tsx
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
|
In addition to the aforementioned issue with the multiple flyouts, I don't think we want to ship with this scenario either (resizing with docs flyout in between) :/ Below, you'll see how resizing the doc viewer gets pretty funky if/when the inline docs panel is sandwiched in between. Perhaps @davismcphee or @jughosta have some ideas given their deep knowledge of Discover and its near-infamous flyout trevails :D Can you all think of any creative solutions with this? As in, is it possible/how painful would it be to close the docs flyout when opening the doc viewer and vice versa?
|
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
|
Responded to @ryankeairns directly in Slack, but adding here too. If the ES|QL editor provides an |
|
@davismcphee how exactly are you thinking to do so? The flyout is rendered from the unified search. You mean adding extra properties to unified search? (and to the language component?) Sounds a bit dirty 🤔 But anyway I don't want to block this PR for this scenario. We can experiment in a follow up PR. The same behavior happens already in main between the docs viewer flyout and the inline editing flyout. So this behavior is not introduced here and we need to think it holistically I think. Maybe something we need to discuss with EUI?
|
## Summary Closes elastic#166907 This PR: 1. Changes the inline docs implementation to a flyout for Discover <img width="1256" alt="image" src="https://github.com/user-attachments/assets/3665c73a-82d5-49b9-88c3-e129eda63885"> 2. Adds the flyout to open from the help menu and removes it from the editor footer 3. For the inline editing changes the docs to be rendered as a component inside the editor (exactly as the history component) Note: The documentation in the inline editing has limited space (same problem has the history component). I need to sync with Ryan to see what we can do, I am exploring some ideas in follow up PRs <img width="647" alt="image" src="https://github.com/user-attachments/assets/2fa9fcc3-2a07-4bea-b675-5ae6578696d3"> 4. Moves the esql docs to the language package and the script associated to this ### Follow up - The language package should be renamed as the `popover` makes sense only for the formula implementation now. I will do it after this PR gets merged - See how we can give more space to the docs / history container when in inline mode ### Note The async bundle got increased as we want the ES|QL docs in unified search too. It wont be a problem as is loaded asynchronously. ### 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 - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Ryan Keairns <contactryank@gmail.com> Co-authored-by: Drew Tate <andrew.tate@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> (cherry picked from commit dfbd7de)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…193444) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Enhances the inline documentation experience (#192156)](#192156) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Stratoula Kalafateli","email":"efstratia.kalafateli@elastic.co"},"sourceCommit":{"committedDate":"2024-09-19T15:10:39Z","message":"[ES|QL] Enhances the inline documentation experience (#192156)\n\n## Summary\r\n\r\nCloses #166907 PR:\r\n\r\n1. Changes the inline docs implementation to a flyout for Discover\r\n\r\n<img width=\"1256\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/3665c73a-82d5-49b9-88c3-e129eda63885\">\r\n\r\n2. Adds the flyout to open from the help menu and removes it from the\r\neditor footer\r\n\r\n3. For the inline editing changes the docs to be rendered as a component\r\ninside the editor (exactly as the history component)\r\n\r\nNote: The documentation in the inline editing has limited space (same\r\nproblem has the history component). I need to sync with Ryan to see what\r\nwe can do, I am exploring some ideas in follow up PRs\r\n\r\n<img width=\"647\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/2fa9fcc3-2a07-4bea-b675-5ae6578696d3\">\r\n\r\n4. Moves the esql docs to the language package and the script associated\r\nto this\r\n\r\n### Follow up\r\n- The language package should be renamed as the `popover` makes sense\r\nonly for the formula implementation now. I will do it after this PR gets\r\nmerged\r\n- See how we can give more space to the docs / history container when in\r\ninline mode\r\n\r\n### Note\r\nThe async bundle got increased as we want the ES|QL docs in unified\r\nsearch too. It wont be a problem as is loaded asynchronously.\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- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Ryan Keairns <contactryank@gmail.com>\r\nCo-authored-by: Drew Tate <andrew.tate@elastic.co>\r\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>","sha":"dfbd7de3f53cae4b81d8643283f106cbf16e3415","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","v9.0.0","backport:prev-minor","Feature:ES|QL","Team:ESQL","v8.16.0"],"title":"[ES|QL] Enhances the inline documentation experience","number":192156,"url":"#192156 Enhances the inline documentation experience (#192156)\n\n## Summary\r\n\r\nCloses #166907 PR:\r\n\r\n1. Changes the inline docs implementation to a flyout for Discover\r\n\r\n<img width=\"1256\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/3665c73a-82d5-49b9-88c3-e129eda63885\">\r\n\r\n2. Adds the flyout to open from the help menu and removes it from the\r\neditor footer\r\n\r\n3. For the inline editing changes the docs to be rendered as a component\r\ninside the editor (exactly as the history component)\r\n\r\nNote: The documentation in the inline editing has limited space (same\r\nproblem has the history component). I need to sync with Ryan to see what\r\nwe can do, I am exploring some ideas in follow up PRs\r\n\r\n<img width=\"647\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/2fa9fcc3-2a07-4bea-b675-5ae6578696d3\">\r\n\r\n4. Moves the esql docs to the language package and the script associated\r\nto this\r\n\r\n### Follow up\r\n- The language package should be renamed as the `popover` makes sense\r\nonly for the formula implementation now. I will do it after this PR gets\r\nmerged\r\n- See how we can give more space to the docs / history container when in\r\ninline mode\r\n\r\n### Note\r\nThe async bundle got increased as we want the ES|QL docs in unified\r\nsearch too. It wont be a problem as is loaded asynchronously.\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- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Ryan Keairns <contactryank@gmail.com>\r\nCo-authored-by: Drew Tate <andrew.tate@elastic.co>\r\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>","sha":"dfbd7de3f53cae4b81d8643283f106cbf16e3415"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"#192156 Enhances the inline documentation experience (#192156)\n\n## Summary\r\n\r\nCloses #166907 PR:\r\n\r\n1. Changes the inline docs implementation to a flyout for Discover\r\n\r\n<img width=\"1256\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/3665c73a-82d5-49b9-88c3-e129eda63885\">\r\n\r\n2. Adds the flyout to open from the help menu and removes it from the\r\neditor footer\r\n\r\n3. For the inline editing changes the docs to be rendered as a component\r\ninside the editor (exactly as the history component)\r\n\r\nNote: The documentation in the inline editing has limited space (same\r\nproblem has the history component). I need to sync with Ryan to see what\r\nwe can do, I am exploring some ideas in follow up PRs\r\n\r\n<img width=\"647\" alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/2fa9fcc3-2a07-4bea-b675-5ae6578696d3\">\r\n\r\n4. Moves the esql docs to the language package and the script associated\r\nto this\r\n\r\n### Follow up\r\n- The language package should be renamed as the `popover` makes sense\r\nonly for the formula implementation now. I will do it after this PR gets\r\nmerged\r\n- See how we can give more space to the docs / history container when in\r\ninline mode\r\n\r\n### Note\r\nThe async bundle got increased as we want the ES|QL docs in unified\r\nsearch too. It wont be a problem as is loaded asynchronously.\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- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Ryan Keairns <contactryank@gmail.com>\r\nCo-authored-by: Drew Tate <andrew.tate@elastic.co>\r\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>","sha":"dfbd7de3f53cae4b81d8643283f106cbf16e3415"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
@stratoula Yup exactly what I'm thinking! Although I wouldn't necessarily consider it dirty unless there's an alternative I'm unaware of 😅 It seems like a fairly standard way to implement it in React IMO. But no worries not blocking this PR over it, it can be looked into as part of #193452 @ryankeairns created. I agree with a holistic solution too (mentioned this to @ryankeairns in Slack as well), and if EUI can help with that, great. Otherwise I'm not above introducing something like a "flyout manager" in Discover and handling it the good old fashion way through props and refs 😁 |
|
Hahaha the fly out manager is a very cool idea. I vote ++ for it. |
Summary
Closes #166907
This PR:
Adds the flyout to open from the help menu and removes it from the editor footer
For the inline editing changes the docs to be rendered as a component inside the editor (exactly as the history component)
Note: The documentation in the inline editing has limited space (same problem has the history component). I need to sync with Ryan to see what we can do, I am exploring some ideas in follow up PRs
Follow up
popovermakes sense only for the formula implementation now. I will do it after this PR gets mergedNote
The async bundle got increased as we want the ES|QL docs in unified search too. It wont be a problem as is loaded asynchronously.
Checklist