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

Move ui/notify banners to New Platform #43610

Merged
merged 7 commits into from
Sep 17, 2019
Merged

Move ui/notify banners to New Platform #43610

merged 7 commits into from
Sep 17, 2019

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Aug 20, 2019 •
edited
Loading

Summary

Fixes #41986
Fixes #44928

This moves the Banner APIs and React Component to Core.

For the time being, the actual rendering of the component is still in the legacy platform. This is left in order to side-step layout problems by moving the rendering outside the app-wrapper parent div.

Dev Docs

Client-side New Platform plugins may now register global banners using the core.overlays.banners API:

core.overlays.banners.add((el: HTMLElement) => {
  ReactDOM.render(<MyBanner />, el);
  return () => ReactDOM.unmountComponentAtNode(el);
});

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@joshdover joshdover added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.5.0 labels Aug 26, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@joshdover joshdover marked this pull request as ready for review August 26, 2019 22:54
@joshdover joshdover requested review from a team as code owners August 26, 2019 22:54
cchaos
cchaos approved these changes Aug 26, 2019
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

SASS file moves lgtm

@elasticmachine
Copy link
Contributor

💔 Build Failed

@joshdover joshdover force-pushed the np/banners branch 2 times, most recently from b117311 to 657f0e0 Compare August 27, 2019 15:34
@elasticmachine
Copy link
Contributor

💔 Build Failed

@joshdover joshdover requested a review from a team as a code owner August 27, 2019 17:06
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

eliperelman
eliperelman approved these changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Looks great!

src/core/public/overlays/banners/banners_list.tsx Outdated Show resolved Hide resolved
src/legacy/ui/public/chrome/chrome.js Outdated Show resolved Hide resolved
@joshdover
Copy link
Contributor Author

Pushed up some updates to run tests before I leave for the weekend. There's still a bit of work to do on testing the priority sorting.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover
Copy link
Contributor Author

Merging so we can this backported for 7.4 BC

@joshdover joshdover merged commit 812c950 into elastic:master Sep 17, 2019
@joshdover joshdover deleted the np/banners branch September 17, 2019 21:12
@joshdover joshdover mentioned this pull request Sep 17, 2019
Merged
joshdover added a commit to joshdover/kibana that referenced this pull request Sep 17, 2019
@joshdover
Move ui/notify banners to New Platform (elastic#43610)
1acd2ba
@joshdover joshdover mentioned this pull request Sep 17, 2019
Merged
joshdover added a commit to joshdover/kibana that referenced this pull request Sep 17, 2019
@joshdover
Move ui/notify banners to New Platform (elastic#43610)
5ed9c31
joshdover added a commit to joshdover/kibana that referenced this pull request Sep 17, 2019
@joshdover
Move ui/notify banners to New Platform (elastic#43610)
1067a78
joshdover added a commit to joshdover/kibana that referenced this pull request Sep 17, 2019
@joshdover
Move ui/notify banners to New Platform (elastic#43610)
f008ae2
@joshdover joshdover mentioned this pull request Sep 17, 2019
Merged
7 tasks
joshdover added a commit that referenced this pull request Sep 18, 2019
@joshdover
[7.4] Move ui/notify banners to New Platform (#43610) (#45958)
fe8e6ce
joshdover added a commit that referenced this pull request Sep 18, 2019
@joshdover
[7.x] Move ui/notify banners to New Platform (#43610) (#45956)
7c29e26 
* Move ui/notify banners to New Platform (#43610)

* Fix Lens component
@joshdover joshdover added v7.4.0 and removed v7.5.0 labels Sep 18, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom lizozom mentioned this pull request Sep 23, 2019
Closed
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mshustov mshustov mshustov left review comments

@eliperelman eliperelman eliperelman approved these changes

@cchaos cchaos cchaos approved these changes

Assignees
No one assigned
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Custom notification banner is not working anymore Add Banners to OverlayService
5 participants
@joshdover @elasticmachine @eliperelman @cchaos @mshustov