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

[Reporting] Make "ScreenCapturePanel" shareable for Canvas #100623

Merged

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented May 26, 2021

Summary

This PR adds code in the Reporting public plugin that is exposed on the plugin contract, for the purpose of sharing with Canvas app and removing redundant UI code from Canvas. The effect of doing this refactoring is fixing UI inconsistencies between Canvas app and Dashboard app.

Closes:

Checklist

Delete any items that are not applicable to this PR.

@tsullivan tsullivan requested a review from a team as a code owner May 26, 2021 01:05
@tsullivan tsullivan marked this pull request as draft May 26, 2021 01:05
@tsullivan tsullivan force-pushed the reporting/canvas-job-notifier-shared branch 2 times, most recently from 4028fbb to 4f70988 Compare May 27, 2021 02:55
@tsullivan tsullivan force-pushed the reporting/canvas-job-notifier-shared branch 5 times, most recently from 6f5cfa0 to 424b165 Compare June 2, 2021 04:38
}

export class ScreenCapturePanelContent extends Component<Props, State> {
constructor(props: Props) {
super(props);

const { objectType } = props.getJobParams();
const isPreserveLayoutSupported = props.reportType !== 'png' && objectType !== 'visualization';
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this since elsewhere in the code it's fine/expected for PNG layout to provide "preserve" layout as the option. Therefore, preserve layout is always supported.

@tsullivan tsullivan force-pushed the reporting/canvas-job-notifier-shared branch 3 times, most recently from 188a0b8 to 0887c3b Compare June 2, 2021 17:16
@tsullivan tsullivan added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:AppServices v7.14.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jun 2, 2021
@tsullivan tsullivan marked this pull request as ready for review June 2, 2021 19:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@tsullivan tsullivan requested review from dokmic, jloleysens and a team June 2, 2021 19:58
@tsullivan tsullivan force-pushed the reporting/canvas-job-notifier-shared branch from 0887c3b to 234c5f5 Compare June 2, 2021 23:14
@tsullivan tsullivan force-pushed the reporting/canvas-job-notifier-shared branch from 234c5f5 to 112fb42 Compare June 2, 2021 23:23
@tsullivan tsullivan requested a review from poffdeluxe June 2, 2021 23:23
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

I still need to finish testing locally, left a couple questions!

return (
<ScreenCapturePanelContent
layoutOption={props.layoutOption}
requiresSavedState={false}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the intention is to make this available to Canvas, but I wonder whether still should not treat this as through it could be placed anywhere (like in Dashboard?). If it is the case that we want to treat it as though it can be used anywhere then I think we should not hardcode this value but rather have the consumer pass it through to us since PDFs, in certain context, do still require saved state. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you it's cleaner to treat this as if it could be used in Dashboard. I don't want to get too deep yet in perfecting the integration touchpoints for the reporting panel. There will be future tech debt / refactoring work to move the export implementation logic out of Reporting, and have new plugins that provide export implementation. That will affect the integration touchpoints, because apps would go from importing this panel component from Reporting, to importing it from a new plugin that controls the implementation detail of the export.

  • current: <services.reporting.components.ReportingPanelPDF />
  • future: <services.exportPDF.components.ReportingPanelPDF />

That is why this PR does not change the Reporting plugin contract further, for example it doesn't add CSV or PNG export panel to the contract.

This is a good note for any reviewer - I'll update the description to talk about this.

cc @ppisljar

Copy link
Member Author

Choose a reason for hiding this comment

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

There will be future tech debt / refactoring work to move the export implementation logic out of Reporting, and have new plugins that provide export implementation

Related Discuss issue: #101422

};

private getLayout = () => {
private handleCanvasLayoutChange = (evt: EuiSwitchEvent) => {
this.setState({ useCanvasLayout: evt.target.checked, usePrintLayout: false });
Copy link
Contributor

@jloleysens jloleysens Jun 4, 2021

Choose a reason for hiding this comment

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

The UX here looks like if the user presses the canvas switch (say it is in the "off" position) it will set usePrintLayout to false. Pressing it again will leave usePrintLayout in false. I guess this leaves in in the "default" layout which would be "X"?

I'm wondering, if these controls are related in this way (cannot choose one while we've chosen the other) whether a button group component or similar might not be more appropriate i.t.o. UX. Open to other thoughts here!

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see I was mistaken on this one. Looking at the private renderOptions function I thought that we would render two toggles in some instances but then I saw the isPrintLayoutSupported and isCanvasLayoutSupported are mutually exclusive values that are determined at component mount.

IMO it would be clearer for readers if we read:

if (this.state.isPrintLayoutSupported) as if (this.props.layoutOption === 'print') and chained the if as else if ... for the subsequent canvas layout check to communicate their mutual exclusivity. This is a nit, won't block on this!

@jloleysens
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

I tested both Canvas and the example reporting plugins locally and both worked as intended. Great work @tsullivan !

I did not in the example plugin the canvas layout option and the pdf layout option linked to an empty panel which made the example a bit incomplete.

Screenshot 2021-06-07 at 11 17 27

Won't block you on this though, but it might be worth updating.

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.

This looks good to me over. One thing that I noticed: if you have reporting enabled but don't have the correct license type and you attempt to generate a PDF, you get a toast notification that just says "Forbidden" where I think previously you'd get a mention about needing to update your license.

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

@tsullivan
Copy link
Member Author

One thing that I noticed: if you have reporting enabled but don't have the correct license type and you attempt to generate a PDF, you get a toast notification that just says "Forbidden" where I think previously you'd get a mention about needing to update your license.

That seems like a regression bug: the feature controls that guard Reporting should be aware of the license as well as the application privileges from the user role. I'll look into it.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
canvas 1090 1089 -1
reporting 49 51 +2
total +1

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
reporting 132 131 -1

Async chunks

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

id before after diff
canvas 1.3MB 1.3MB -4.7KB

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
reporting 19 18 -1

Page load bundle

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

id before after diff
canvas 534.7KB 530.9KB -3.7KB
reporting 84.4KB 85.5KB +1.1KB
total -2.6KB
Unknown metric groups

API count

id before after diff
reporting 133 132 -1

History

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

@tsullivan tsullivan merged commit 3559d37 into elastic:master Jun 10, 2021
@tsullivan tsullivan deleted the reporting/canvas-job-notifier-shared branch June 10, 2021 00:08
tsullivan added a commit to tsullivan/kibana that referenced this pull request Jun 10, 2021
…00623)

* use ScreenCapturePanel component in Canvas

* use smaller state object

* add comment about canvas-specific shared component

* fix example

* fix toast error

* fix i18n

* fix data-test-subj

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
tsullivan added a commit that referenced this pull request Jun 10, 2021
…0623) (#101852)

* [Reporting] Make "ScreenCapturePanel" shareable for Canvas (#100623)

* use ScreenCapturePanel component in Canvas

* use smaller state object

* add comment about canvas-specific shared component

* fix example

* fix toast error

* fix i18n

* fix data-test-subj

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* fix compatibility type ts

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 10, 2021
…add-agent-flyout

* 'master' of github.com:elastic/kibana: (35 commits)
  [Cases] Improve connectors mapping (elastic#101145)
  [ML] Fixes display of job group badges in recognizer wizard (elastic#101775)
  Fix es_archives path (elastic#101737)
  [kbnArchiver] convert archive names to root-relative paths (elastic#101839)
  [Reporting] Make "ScreenCapturePanel" shareable for Canvas (elastic#100623)
  [Alerting UI] Converted Rules and Connectors management pages to new layout. (elastic#101697)
  [Fleet] Support granular integrations in policy editor (elastic#101531)
  [Security Solution][Detections] Update detection alert mappings to ECS v1.10.0 (elastic#101680)
  [Fleet] Integrations UI: Adjust policies list UI (elastic#101600)
  chore(NA): moving @kbn/server-route-repository into bazel (elastic#101484)
  Support owner and description attributes inside the Manifest file, use in API docs (elastic#101786)
  [Security Solution] fix security empty overview links (elastic#101536)
  Unskips migration tests now that elastic search is fixed (elastic#101682)
  Fix endpoint -> integrations onboarding link (elastic#101804)
  [Alerting] Log warning when rules are not rescheduled due to Saved Object not found error (elastic#101591)
  Update datafeed_high_count_network_denies.json (elastic#101681)
  [Index patterns] Field editor example app (elastic#100524)
  [DOCS] Adding file upload to add data page (elastic#101674)
  [Security Solution][Endpoint] Adds Endpoint Host Isolation Status common component (elastic#101782)
  Upgrade ws v7.3.1->v7.4.2 and v6.2.1->v6.2.2 (elastic#101402)
  ...

# Conflicts:
#	x-pack/plugins/fleet/public/components/agent_enrollment_flyout/agent_policy_selection.tsx
#	x-pack/plugins/fleet/public/components/agent_enrollment_flyout/index.tsx
#	x-pack/plugins/fleet/public/components/agent_enrollment_flyout/managed_instructions.tsx
#	x-pack/plugins/fleet/public/components/agent_enrollment_flyout/standalone_instructions.tsx
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 10, 2021
…add-integrations-redirect

* 'master' of github.com:elastic/kibana: (44 commits)
  Allow navigating discover flyout via arrow keys (elastic#101772)
  [Cases] Improve connectors mapping (elastic#101145)
  [ML] Fixes display of job group badges in recognizer wizard (elastic#101775)
  Fix es_archives path (elastic#101737)
  [kbnArchiver] convert archive names to root-relative paths (elastic#101839)
  [Reporting] Make "ScreenCapturePanel" shareable for Canvas (elastic#100623)
  [Alerting UI] Converted Rules and Connectors management pages to new layout. (elastic#101697)
  [Fleet] Support granular integrations in policy editor (elastic#101531)
  [Security Solution][Detections] Update detection alert mappings to ECS v1.10.0 (elastic#101680)
  [Fleet] Integrations UI: Adjust policies list UI (elastic#101600)
  chore(NA): moving @kbn/server-route-repository into bazel (elastic#101484)
  Support owner and description attributes inside the Manifest file, use in API docs (elastic#101786)
  [Security Solution] fix security empty overview links (elastic#101536)
  Unskips migration tests now that elastic search is fixed (elastic#101682)
  Fix endpoint -> integrations onboarding link (elastic#101804)
  [Alerting] Log warning when rules are not rescheduled due to Saved Object not found error (elastic#101591)
  Update datafeed_high_count_network_denies.json (elastic#101681)
  [Index patterns] Field editor example app (elastic#100524)
  [DOCS] Adding file upload to add data page (elastic#101674)
  [Security Solution][Endpoint] Adds Endpoint Host Isolation Status common component (elastic#101782)
  ...

# Conflicts:
#	x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/index.tsx
#	x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/details_page/components/package_policies/package_policies_table.tsx
semd pushed a commit that referenced this pull request Jun 10, 2021
* use ScreenCapturePanel component in Canvas

* use smaller state object

* add comment about canvas-specific shared component

* fix example

* fix toast error

* fix i18n

* fix data-test-subj

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants