[core/public/banners] migrate ui/notify/banners #23215
Conversation
spalger
added
Team:Core
💚 Build Succeeded |
|
ack: on community duty today, will take a look today or tomorrow at the latest. |
src/core/public/notifications/banners/global_banner_list.test.tsx
Outdated
Show resolved
Hide resolved
src/core/public/notifications/banners/global_banner_list.test.tsx
Outdated
Show resolved
Hide resolved
| return ( | ||
| <div className="globalBanner__list"> | ||
| {banners.map(({ id, component, priority, ...rest }) => ( | ||
| <div key={id} className="globalBanner__item" {...rest}> |
There was a problem hiding this comment.
question: Is there a plan to add this stuff to EUI?
There was a problem hiding this comment.
I don't think so, the banners themselves are really what I think we should focus on moving to EUI, but as you see here they are passed in fully styled. I tried moving the divs here to use <EuiFlexGroup />, but since we need refs we have issues there, so I didn't think it was worth the effort right now.
spalger
force-pushed
the
migrate-new-platform/global-banners
branch
from
9956a17 to
fc52d56
Compare
spalger
force-pushed
the
migrate-new-platform/global-banners
branch
from
7b9f49b to
d90a6e7
Compare
💔 Build Failed |
|
CI is failing because of failed |
💔 Build Failed |
|
|
||
| export const banners = { | ||
| /** | ||
| * Add a new banner. | ||
| * | ||
| * @param {Object} component The React component to display. |
There was a problem hiding this comment.
optional nit: JSDoc here and in set is completely invalid, there is a banner argument with component property, optional priority argument, no need in JSDoc types etc., but since it was here before feel free to keep it as is.
| @@ -1,360 +0,0 @@ | |||
| # Banners | |||
There was a problem hiding this comment.
question: is this README completely irrelevant now so that we remove it?
| notifications.start(); | ||
| notifications.stop(); | ||
|
|
||
| expect(BannersService.prototype.stop).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
question: what is the value in these two toHaveBeenCalled.... asserts? It seems what you want to check will be enforced by the expect.assertions(...); anyway?
| export { Banners, Banner, BannersService, BannersStartContract } from './banners_service'; |
There was a problem hiding this comment.
question: do you really need to re-export anything except for BannersService and BannersStartContract?
| private unrenderBanner?: () => void; | ||
|
|
||
| public componentDidMount() { | ||
| if (!this.ref.current) { |
There was a problem hiding this comment.
optional nit: we repeat very similar piece of code twice, would it make sense to move some logic into, let's say, renderBanner and reuse it in both componentDidMount and componentDidUpdate?
private renderBanner() {
if (this.unrenderBanner) {
this.unrenderBanner();
}
if (!this.ref.current) {
throw new Error('<GlobalBanner /> mounted without ref');
}
this.unrenderBanner = this.props.banner.render(this.ref.current) || undefined;
}| .globalBanner__list { | ||
| padding: 16px; | ||
| } | ||
|
|
||
| .globalBanner__item + .globalBanner__item { |
There was a problem hiding this comment.
note: wondering how bad is just .globalBanner__item { margin-botton: 16px; } so that we resort to +...
|
|
||
| import { Banners } from '../banners_service'; | ||
| import { GlobalBannerList } from '../components/global_banner_list'; | ||
| import './global_banner_list.css'; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const component = shallow(<GlobalBannerList banners={[]} />); | ||
| expect(component).toMatchInlineSnapshot(` | ||
| <div | ||
| className="globalBanner__list" |
There was a problem hiding this comment.
question: className looks weird, should it be class?
There was a problem hiding this comment.
Not yet facebook/react#13525
|
Code looks good, I want to test it locally though. Also CI failures look legit this time, could you please check? |
💔 Build Failed |
|
Okay, I was totally wrong about the need to do this right now. Banners are actually shown inside the app container (not outside the chrome) so we can't even do this until we start rendering the chrome via the new platform. Because of that I'm going to close this for now so I'm closing this until later. |
Part of #19992, in order for the banners to be rendered above the navigation when we move the chrome out of the legacy platform the banners need to also be rendered by the new platform. The API hasn't changed at all.