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

[Lens] Inline editing of lens panels on a dashboard or canvas #166169

Merged
merged 59 commits into from
Sep 27, 2023

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Sep 11, 2023

Summary

Closes #166833
Closes #167063

This PR allows the inline editing of Lens panels in canvas and dashboards.

inline_editing

Changes

Discover

  • The basic UI change in Discover flyout is that we add the Cancel button in the flyout footer (resets to the state before the flyout changes).
  • It also solves (with passing the embeddableOutput observable to the flyout) the bug that is more prominent in this PR (first bullet)

Dashboard

  • All Lens panels have an Edit visualization panel action
  • The Edit Lens action is now hidden
  • A flyout opens (in the header there is a link where the users can go to the editor)
  • The by reference panels change to by value as long as the users are editing. On apply and close, we save to the SO
  • All tests have changed to accomodate this navigation change. All the existing ones test the editor (as they used to do). I have added new tests for testing the inline experience

Known issues

  • If a user is currently editing a by reference visualization, doesn't click Cancel or Apply and navigate away, the panel stays as by value panel. The users can always go back and reset the changes and we could not find an easy fix with Devon. We consider it as a nice to have and a non-blocking behavior.

Missing

These 2 features are missing to give the full set of capabilities of the editor to the flyout. Both are missing from ESQL editing too and are going to be added on a follow up PR

  • Chart switcher
  • Suggestions (as icons and not previews)

Flaky tests runner

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3182 (100 times)

Panel focus

This PR will look even better when this is merged! #165417

Checklist

@@ -38,24 +38,6 @@ describe('open config panel action', () => {
expect(isCompatible).toBeFalsy();
});

it('is incompatible with non text based language embeddable', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ no need for this test now that we enable inline editing everywhere.

@@ -75,6 +75,7 @@ export class EditPanelAction implements Action<ActionContext> {
const canEditEmbeddable = Boolean(
embeddable &&
embeddable.getOutput().editable &&
!embeddable.getOutput().inlineEditable &&
(embeddable.getOutput().editUrl ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ hides the embeddable edit action if inline editing exists

@@ -69,7 +73,8 @@ export interface ChartProps {
disableTriggers?: LensEmbeddableInput['disableTriggers'];
disabledActions?: LensEmbeddableInput['disabledActions'];
input$?: UnifiedHistogramInput$;
lensTablesAdapter?: Record<string, Datatable>;
lensAdapters?: UnifiedHistogramChartLoadEvent['adapters'];
Copy link
Contributor Author

@stratoula stratoula Sep 19, 2023

Choose a reason for hiding this comment

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

ℹ️ Instead of sending only the tables we now send all the adapters object to Lens flyout. With this way we can fetch correctly the active data. (Solves a bug currently in main more visible on this PR, check first bullet here

@@ -83,7 +83,7 @@ export default function canvasLensTest({ getService, getPageObjects }: FtrProvid
const panelHeader = await testSubjects.find('embeddablePanelHeading-');
await dashboardPanelActions.openContextMenu(panelHeader);
await dashboardPanelActions.clickEdit();
await await PageObjects.lens.saveAndReturn();
await PageObjects.lens.saveAndReturn();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ Irrelevant with this change, I just saw it and decided to fix

@elastic elastic deleted a comment from kibana-ci Sep 25, 2023
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Functional test changes + small Embeddables changes LGTM! Also tested this out in chrome, and it feels really snappy and nice. What a great addition to the product!

One small nit, the labels in the editor make reference to drag and drop a field but there is no where to drag fields from!

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Unified Histogram changes look good to me overall! I left a small nit, but nothing blocking. The "Cancel" and "Apply and close" buttons are definitely an improvement in Discover too.

The only issue I ran into when testing in Discover was that for some reason I couldn't interact with half the screen whenever the Lens flyout was open. Maybe some styles were introduced in this PR that caused it?

In main:
working

In this branch:
broken

src/plugins/unified_histogram/public/chart/histogram.tsx Outdated Show resolved Hide resolved
@stratoula stratoula requested a review from a team September 26, 2023 08:01
@stratoula
Copy link
Contributor Author

@ThomThomson thanx a lot for your review. You can drag and drop fields from other dimensions so I don't think it is wrong
tbh 🤔

If you think that it is confusing though I can discuss with our ux writters!

Screenshot 2023-09-26 at 11 04 05 AM (2)

@stratoula
Copy link
Contributor Author

stratoula commented Sep 26, 2023

The only issue I ran into when testing in Discover was that for some reason I couldn't interact with half the screen whenever the Lens flyout was open. Maybe some styles were introduced in this PR that caused it?

@davismcphee good catch! Fixed!

@stratoula
Copy link
Contributor Author

The type check fails due to problems that exist in main and are not introduced by this PR

Copy link
Member

@sphilipse sphilipse left a comment

Choose a reason for hiding this comment

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

ent-search changes LGTM

@ThomThomson
Copy link
Contributor

If you think that it is confusing though I can discuss with our ux writters!

Maybe this is just a symptom of the way I use Lens - I always drag fields in from the left. It could be worth re-visiting the copy, but I'd defer to you on it!

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

The Discover flyout issue is fixed! Thanks, LGTM 👍

@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 27, 2023

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #10 / serverless common UI Avatar menu "before all" hook for "is displayed"

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
embeddable 435 436 +1
lens 520 524 +4
total +5

Async chunks

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

id before after diff
enterpriseSearch 2.6MB 2.6MB -2.0B
lens 1.3MB 1.4MB +5.6KB
unifiedHistogram 48.6KB 48.9KB +268.0B
total +5.9KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
lens 61 60 -1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
embeddable 77.7KB 77.7KB +31.0B
Unknown metric groups

API count

id before after diff
embeddable 535 536 +1
lens 617 622 +5
unifiedHistogram 53 55 +2
total +8

References to deprecated APIs

id before after diff
lens 112 115 +3

History

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

@delanni delanni merged commit 784411d into elastic:main Sep 27, 2023
amyjtechwriter added a commit that referenced this pull request Nov 6, 2023
## Summary

With the arrival of inline editing to Lens panels the [Create
visualizations](https://www.elastic.co/guide/en/kibana/current/lens.html#create-the-visualization-panel)
'Edit and delete' section has been updated. Users can now use the option
**Edit visualization** to make edits to Lens visualizations using a
flyout panel, without having to leave the dashboard and go into the Lens
application.

The [Edit
panels](https://www.elastic.co/guide/en/kibana/current/dashboard.html#edit-panels)
section on the 'Dashboards and visualizations' page has been edited as
the **Edit Lens** option has been removed from the UI.

Relates to: #166169 &
[#243](https://github.com/elastic/platform-docs-team/issues/243)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 6, 2023
## Summary

With the arrival of inline editing to Lens panels the [Create
visualizations](https://www.elastic.co/guide/en/kibana/current/lens.html#create-the-visualization-panel)
'Edit and delete' section has been updated. Users can now use the option
**Edit visualization** to make edits to Lens visualizations using a
flyout panel, without having to leave the dashboard and go into the Lens
application.

The [Edit
panels](https://www.elastic.co/guide/en/kibana/current/dashboard.html#edit-panels)
section on the 'Dashboards and visualizations' page has been edited as
the **Edit Lens** option has been removed from the UI.

Relates to: elastic#166169 &
[elastic#243](https://github.com/elastic/platform-docs-team/issues/243)

(cherry picked from commit 5e84147)
kibanamachine added a commit that referenced this pull request Nov 6, 2023
# Backport

This will backport the following commits from `main` to `8.11`:
- [[DOCS] Inline editing in Lens
(#170649)](#170649)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT
[{"author":{"name":"amyjtechwriter","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-11-06T15:25:03Z","message":"[DOCS]
Inline editing in Lens (#170649)\n\n## Summary\r\n\r\nWith the arrival
of inline editing to Lens panels the
[Create\r\nvisualizations](https://www.elastic.co/guide/en/kibana/current/lens.html#create-the-visualization-panel)\r\n'Edit
and delete' section has been updated. Users can now use the
option\r\n**Edit visualization** to make edits to Lens visualizations
using a\r\nflyout panel, without having to leave the dashboard and go
into the Lens\r\napplication.\r\n\r\nThe
[Edit\r\npanels](https://www.elastic.co/guide/en/kibana/current/dashboard.html#edit-panels)\r\nsection
on the 'Dashboards and visualizations' page has been edited as\r\nthe
**Edit Lens** option has been removed from the UI.\r\n\r\nRelates to:
#166169
&\r\n[#243](https://github.com/elastic/platform-docs-team/issues/243)","sha":"5e8414779ae60b2d8e7e9a5019b78045c1ce14a2","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Docs","Team:Visualizations","release_note:skip","Feature:Lens","docs","v8.11.0","v8.12.0"],"number":170649,"url":"https://github.com/elastic/kibana/pull/170649","mergeCommit":{"message":"[DOCS]
Inline editing in Lens (#170649)\n\n## Summary\r\n\r\nWith the arrival
of inline editing to Lens panels the
[Create\r\nvisualizations](https://www.elastic.co/guide/en/kibana/current/lens.html#create-the-visualization-panel)\r\n'Edit
and delete' section has been updated. Users can now use the
option\r\n**Edit visualization** to make edits to Lens visualizations
using a\r\nflyout panel, without having to leave the dashboard and go
into the Lens\r\napplication.\r\n\r\nThe
[Edit\r\npanels](https://www.elastic.co/guide/en/kibana/current/dashboard.html#edit-panels)\r\nsection
on the 'Dashboards and visualizations' page has been edited as\r\nthe
**Edit Lens** option has been removed from the UI.\r\n\r\nRelates to:
#166169
&\r\n[#243](https://github.com/elastic/platform-docs-team/issues/243)","sha":"5e8414779ae60b2d8e7e9a5019b78045c1ce14a2"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/170649","number":170649,"mergeCommit":{"message":"[DOCS]
Inline editing in Lens (#170649)\n\n## Summary\r\n\r\nWith the arrival
of inline editing to Lens panels the
[Create\r\nvisualizations](https://www.elastic.co/guide/en/kibana/current/lens.html#create-the-visualization-panel)\r\n'Edit
and delete' section has been updated. Users can now use the
option\r\n**Edit visualization** to make edits to Lens visualizations
using a\r\nflyout panel, without having to leave the dashboard and go
into the Lens\r\napplication.\r\n\r\nThe
[Edit\r\npanels](https://www.elastic.co/guide/en/kibana/current/dashboard.html#edit-panels)\r\nsection
on the 'Dashboards and visualizations' page has been edited as\r\nthe
**Edit Lens** option has been removed from the UI.\r\n\r\nRelates to:
#166169
&\r\n[#243](https://github.com/elastic/platform-docs-team/issues/243)","sha":"5e8414779ae60b2d8e7e9a5019b78045c1ce14a2"}}]}]
BACKPORT-->

Co-authored-by: amyjtechwriter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Embedding Embedding content via iFrame Feature:Lens release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.11.0
Projects
None yet
8 participants