Skip to content

[CANVAS] Moves notify to a canvas service#63268

Merged
crob611 merged 7 commits intoelastic:masterfrom
crob611:canvas-notify-service
Apr 24, 2020
Merged

[CANVAS] Moves notify to a canvas service#63268
crob611 merged 7 commits intoelastic:masterfrom
crob611:canvas-notify-service

Conversation

@crob611
Copy link
Copy Markdown
Contributor

@crob611 crob611 commented Apr 10, 2020

This moves what was previously lib/notify, which has a dependency on core.notifications into a "service".

Services will start and stop as part of the Canvas mount/unmount process.

The services are also added onto the top level React Context, so they are easily accessible by components.

@crob611 crob611 added review Feature:New Platform Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// loe:medium Medium Level of Effort v8.0.0 release_note:skip Skip the PR/issue when compiling release notes impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. v7.8.0 labels Apr 10, 2020
@crob611 crob611 requested a review from a team as a code owner April 10, 2020 18:38
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Apr 10, 2020

Pinging @elastic/kibana-canvas (Team:Canvas)

@crob611
Copy link
Copy Markdown
Contributor Author

crob611 commented Apr 13, 2020

@elasticmachine merge upstream

@crob611
Copy link
Copy Markdown
Contributor Author

crob611 commented Apr 16, 2020

@elasticmachine merge upstream

@crob611
Copy link
Copy Markdown
Contributor Author

crob611 commented Apr 22, 2020

@elasticmachine merge upstream

poffdeluxe
poffdeluxe approved these changes Apr 23, 2020
View reviewed changes
Copy link
Copy Markdown
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.

looks good -- minor questions. One thing you'll want to check (if you haven't already) is both of the canvas storybook sets. I'm pretty sure lib/notify is a module we mock out manually via module replacement

x-pack/legacy/plugins/canvas/public/components/asset_manager/index.ts
const mergeProps = (stateProps, dispatchProps, ownProps) => {
const mergeProps = (
stateProps: ReturnType<typeof mapStateToProps>,
dispatchProps: ReturnType<typeof mapDispatchToProps>,
Copy link
Copy Markdown
Contributor

@poffdeluxe poffdeluxe Apr 23, 2020

Choose a reason for hiding this comment

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

handy

x-pack/legacy/plugins/canvas/public/components/asset_manager/index.ts
};

export const AssetManager = compose(
export const AssetManager = compose<any, any>(
Copy link
Copy Markdown
Contributor

@poffdeluxe poffdeluxe Apr 23, 2020

Choose a reason for hiding this comment

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

can we get types here?

@crob611
Copy link
Copy Markdown
Contributor Author

crob611 commented Apr 24, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 24, 2020

💚 Build Succeeded

History

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

@crob611 crob611 merged commit 4051c94 into elastic:master Apr 24, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 27, 2020
@jloleysens
Merge branch 'feature/ingest-node-pipelines' of github.com:elastic/ki…
42247e2 
…bana into ingest-node-pipeline/open-flyout-create-edit

* 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (116 commits)
  [Ingest Node Pipelines] More lenient treatment of on-failure value (elastic#64411)
  Report Deletion via UI- functional test (elastic#64031)
  Bump handlebars dependency from 4.5.3 to 4.7.6 (elastic#64402)
  [Uptime] Update TLS settings (elastic#64111)
  [alerting] removes usage of any throughout Alerting Services code (elastic#64161)
  [CANVAS] Moves notify to a canvas service (elastic#63268)
  [Canvas] Misc NP Stuff (elastic#63703)
  update apm index pattern (elastic#64232)
  Task/hostlist pagination (elastic#63722)
  [NP] Vega migration (elastic#63849)
  Move ensureDefaultIndexPattern into data plugin (elastic#63100)
  [Fleet] Fix agent status count to not include unenrolled agents (elastic#64106)
  Migrate graph_workspace saved object registration to Kibana platform (elastic#64157)
  Index pattern management UI -> TypeScript and New Platform Ready (edit_index_pattern) (elastic#64184)
  [ML] EuiDataGrid ml/transform components. (elastic#63447)
  [ML] Moving to kibana capabilities (elastic#64057)
  Move input_control_vis into NP (elastic#63333)
  remove reference to local application service in graph (elastic#64288)
  KQL removes leading zero and breaks query (elastic#62748)
  [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 28, 2020
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 28, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 63268 or prevent reminders by adding the backport:skip label.

@crob611 crob611 mentioned this pull request Apr 28, 2020
Merged
crob611 pushed a commit to crob611/kibana that referenced this pull request Apr 28, 2020
[CANVAS] Moves notify to a canvas service (elastic#63268)
3a4b187 
* Moves notify to a canvas service

* Typecheck fix
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 29, 2020

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 29, 2020
crob611 pushed a commit that referenced this pull request Apr 29, 2020
@elasticmachine
[CANVAS] Moves notify to a canvas service (#63268) (#64666)
1717736 
* Moves notify to a canvas service

* Typecheck fix

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

Reviewers

1 more reviewer

@poffdeluxe poffdeluxe poffdeluxe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:New Platform impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v7.8.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@crob611 @elasticmachine @kibanamachine @poffdeluxe