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.

Sign up for GitHub

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

Jump to bottom

[Lens] Export lens save modal #100381

Merged
merged 44 commits into from
Jun 15, 2021
Merged

[Lens] Export lens save modal #100381

merged 44 commits into from
Jun 15, 2021

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented May 20, 2021 •
edited
Loading

Summary

fix #94585

Export lens save modal functionality so that it can be used outside lens

SaveModalComponent is exported from Lens that can be used outside lens to trigger save modal.

image

@shahzad31 shahzad31 requested a review from flash1293 June 1, 2021 08:02
flash1293
flash1293 reviewed Jun 2, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

It looks like this PR exposes two parts:

  • attributes service via contract
  • static modal export which expects to get the attributes services passed via react context

This seems like more complexity than necessary, can't we just expose the modal via contract with a reference to the attributes service baked in? This way the user doesn't have to take care of two different flavors of lens export which have to be mixed together afterwards.

poffdeluxe
poffdeluxe approved these changes Jun 8, 2021
View reviewed changes
Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

LGTM

flash1293
flash1293 requested changes Jun 9, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

When saving a lens vis to the library only, the id is put into the URL, then disappearing again. Reloading the page will bring you to an empty editor.

  • Go to Lens
  • Configure chart
  • Save to library (no dashboard)
  • Reload page
  • Should re-render with saved vis

@shahzad31 shahzad31 requested a review from flash1293 June 9, 2021 16:23
@shahzad31
Copy link
Contributor Author

@flash1293 good catch, fixed an unnecessary redirect call back call.

flash1293
flash1293 requested changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Not sure whether this broke with the recent change, but the example is not actually navigating to dashboard:

  • Click Save visualization
  • Select new dashboard
  • Click save
  • Toast appears and it stays on example app instead of navigating to dashboard

@shahzad31 shahzad31 requested a review from flash1293 June 10, 2021 16:36
@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

flash1293
flash1293 reviewed Jun 15, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and everything worked fine, great work!

Left one comment - I think there are two ways how we can resolve it:

  • Omit the spinner for these async loadings
  • Render the spinner in an EuiOverlayMask so it will change from a fullscreen spinner to a fullscreen modal

x-pack/plugins/lens/public/app_plugin/shared/saved_modal_lazy.tsx Outdated
}, []);

if (!lensServices) {
return <EuiLoadingSpinner />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the loading spinner components here and in line 20 - as the real component is an EuiModal it will be rendered absolutely positioned while the loading spinner will be inline which is probably not what the user expects: (note the spinner appearing below the chart - depending on the layout it could lead to jumping content)
Kapture 2021-06-15 at 11 21 13

@shahzad31 shahzad31 requested a review from flash1293 June 15, 2021 15:06
flash1293
flash1293 approved these changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and LGTM, thanks for adding this

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 687 689 +2

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
lens 161 162 +1

Async chunks

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

id before after diff
lens 1.4MB 1.5MB +24.2KB
observability 463.3KB 464.2KB +927.0B
total +25.1KB

Page load bundle

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

id before after diff
lens 38.1KB 40.0KB +1.9KB
Unknown metric groups

API count

id before after diff
lens 172 174 +2

async chunk count

id before after diff
lens 2 3 +1

History

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

cc @shahzad31

@shahzad31 shahzad31 added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 15, 2021
@shahzad31 shahzad31 merged commit 264339e into elastic:master Jun 15, 2021
@shahzad31 shahzad31 deleted the export-lens-save-modal branch June 15, 2021 18:06
@kibanamachine kibanamachine mentioned this pull request Jun 15, 2021
Merged
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 15, 2021
@shahzad31 @kibanamachine
[Lens] Export lens save modal (elastic#100381)
71e5515 
Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 15, 2021
@kibanamachine @shahzad31
[Lens] Export lens save modal (#100381) (#102258)
083c2ea 
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Shahzad <[email protected]>
cuff-links pushed a commit to cuff-links/kibana that referenced this pull request Jun 15, 2021
@shahzad31 @kibanamachine
[Lens] Export lens save modal (elastic#100381)
f54d66d 
Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flash1293 flash1293 flash1293 approved these changes

@dej611 dej611 dej611 approved these changes

@poffdeluxe poffdeluxe poffdeluxe approved these changes

Assignees

@shahzad31 shahzad31

Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.14.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Lens] Expose save dialog behavior via plugin contract
6 participants
@shahzad31 @elasticmachine @kibanamachine @poffdeluxe @dej611 @flash1293