Add BannerList and Banner components to display cluster alerts on load #1169
Conversation
There was a problem hiding this comment.
couple questions:
- do we want to show this banner every time the user refreshes the browser?
- do we want to show this banner in the discovery screen?
i think we can re-use our banner in our enterprise by refactoring a little bit, something like this maybe? and then we can write stories to showcase all possible kinds of banners?
OSS:
export default Container() {
const {alerts} = useBanner()
const banners: JSX.Element[] = [];
if (alerts) {
// do stuff
}
return <Banner banners={banners}/>
}Enterprise:
export default Container() {
const ctx = useTeleport();
const {alerts, license} = useBanner()
const banners: JSX.Element[] = [];
if (alerts) {
// do stuff
}
if (license) {
banners.push(
<LicenseWarning
key="license-warning-banner"
severity={license.severity}
text={license.text}
/>
);
}
if (ctx.storeAccessRequests.getSessionExpiry()) {
banners.push( <SwitchBack key="access-request-banner" data-testid="banner" /> );
}
return <Banner banners={banners} />
}Banner.tsx
export const Banner: React.FC = ({ banners, children }) => {
return (
<BannersWrapper banners={banners.length}>
{banners}
{children}
</BannersWrapper>
);
};
const BannersWrapper = styled(Box)`
${MainContainer} {
height: calc(100% - ${props => props.banners * 48}px);
}
min-width: 1000px;
`;
|
Thanks for the reviews, it's ready for another look
Yes these warnings are only dismissible for the current users session. In the future we expect this to change and have different criteria depending on the message (see below).
I didn't think so as I felt the discover UI is a modal over the Access Manager UI.
The enterprise-only handling of the license warning will not be special in the near future as it will instead come over the same cluster alerts endpoint. This warning will also likely not be dismissible. The RFD for this is not yet finalized so we'll modify this functionality when the time comes. |
| banners.forEach(banner => (newList[banner.id] = banner)); | ||
| Object.assign(newList, bannerList); | ||
| setBannerList(newList); | ||
| }, [banners]); |
There was a problem hiding this comment.
What will happen if we fetch alerts for one cluster and then the user will switch to another cluster? Will it display the merged list of alerts from two clusters? Is it intended?
There was a problem hiding this comment.
That is what would happen, yeah.
We don't have a spec as to what's supposed to happen in all of these various notification edge cases. I'd like to see this notification system be replaced by a more fully featured notification system which could filter and categorize the notifications by location/type. For now though I just wanted it to merge any new notifications with ones that were already there.
There was a problem hiding this comment.
if our intention is to merge previous alerts from different clusters, then we need to persist these alerts somehow. won't all the scenarios below remove the previous alerts?
- user refreshes the browser e.g. refreshing a data list, it's probably very commonly done
- in app refresh e.g. assuming an access request triggers a refresh
- opening a route in new tab (i do this often)
- user copies and pastes a link to a different area of the app
persisting alerts will also allow us to persist user action such as closing an alert because i would think it'd be annoying to keep having the same alert you closed pop up after a refresh
There was a problem hiding this comment.
I feel like expanding this functionality is out of scope of this PR but we can definitely work on improving this by adding persistence support in a follow-up.
Lets wait on @xinding33 input, because currently we have banners showing in the discover screen.
Regardless of the license warning, we still have the switchback banner... I suggested this to re-use the banner styling and have banners be placed in one area for a component. Currently in the enterprise version, we have a Main that is wrapped with a banner component, and then your changes adds another banner placement inside Main, so that's like two banner placements which is a bit odd? |
|
Totally agree it's a bit odd, but it's also not finished :D I need these changes to land so that I can make the necessary updates into e |
|
e now has a pr as well and will need to land before this one. |
There was a problem hiding this comment.
i keep finding visual discrepancy, i would suggest to do the following test per change:
Open up a cluster side by side with your dev cluster, each with same setting and I would compare the changes you made and see if it deviates from the original with the following:
- view the page with only the expired license banner, does any behavior here deviate from the original? from my testing, your changes allows the license warning to be closable, but from previous discussions we did not want this alert to be closable. without any explanation or confirmation that this was okay, i'm going to assume that this is a bug
- view the page with only the access request banner
- view the page with only the other alerts
- view the page with all 3 alerts (license, switchback, and one other alert)
test shrink the window size both horizontally and vertically in all above scenarios to catch any visual bugs. from my testing, shrinking horizontally, alerts with long text message will wrap which with your changes, increases the height of the banner, resulting in the very same issue we were trying to fix: double vertical scrolling.
now that we can get various alerts, we might not be able to assume a certain text length anymore. granted the way we did for license wasn't very fool proof either, like the license warning could get longer for whatever reason and it could overflow. neither case is good and it should be fixed.
i think the fastest and easiest way to handle this is to allow the banner to be scrollable, and i think this is okay for now b/c the alert system is still in development (and we aren't expecting long alert messages anytime soon).
the best way would probably be to redesign this whole alert system (or our whole layout) because it has one huge flaw: because our banners are fixed to the top, stacked alerts will reduce the users vertical viewing space. maybe it's not an issue since users can close most of them? but it could be better designed @xinding33 @rishibarbhaya-design
| banners.forEach(banner => (newList[banner.id] = banner)); | ||
| Object.assign(newList, bannerList); | ||
| setBannerList(newList); | ||
| }, [banners]); |
There was a problem hiding this comment.
if our intention is to merge previous alerts from different clusters, then we need to persist these alerts somehow. won't all the scenarios below remove the previous alerts?
- user refreshes the browser e.g. refreshing a data list, it's probably very commonly done
- in app refresh e.g. assuming an access request triggers a refresh
- opening a route in new tab (i do this often)
- user copies and pastes a link to a different area of the app
persisting alerts will also allow us to persist user action such as closing an alert because i would think it'd be annoying to keep having the same alert you closed pop up after a refresh
There was a problem hiding this comment.
in addition, lets also fix the below:
- on switching to discover view, or refershing the page, the dismissed alert flashes before disappearing
- license banner is dismissable, but is not persisted, if we don't intend to make it dismissable then perhaps make it undismissable? (don't render the x button)
- if you shrink the discover page with a banner, there is a wide gap in width on the far right. this is because the banner is smaller than the discover width. this is temporary fixed in v10 by passing in the width, ryan is working on a PR to unify the minWidth, but until then since this PR has to be backported, we should just patch it for now.
| function getItem(key: string): string | null { | ||
| return window.localStorage.getItem(key); | ||
| } | ||
|
|
||
| function setItem(key: string, data: string) { | ||
| window.localStorage.setItem(key, data); | ||
| } |
There was a problem hiding this comment.
we have a local storage file, uses a particular pattern such as getXXX returns already the parsed data: https://github.com/gravitational/webapps/blob/master/packages/teleport/src/services/localStorage/localStorage.ts
There was a problem hiding this comment.
This file doesn't appear to have these utility methods? Or did you want me to add them?
These functions were simply created to save typing :) they don't do anything special.
As the data and deny list come in from different requests I worried that this might happen if they come in out of order. I'll add some gating around this.
It's not persisted because it's injected after the fact so it's not being controlled by the alert system. It will be part of the alert system in the next release though so I'm hesitant to add too much special handling here that we'll have to be sure we go back and remove before the next release. I think I'll modify the data structure to include the
Can you provide me with the PR that contains this patch? |
This reverts commit 0518eea.
#1169) * Add BannerList and Banner components.
When loading the UI, render cluster alerts.
Currently the only supported alert is for an upgrade notification but this component should handle rendering any alerts with multiple severity levels without updates.
QA Notes
When dismissing a banner in the Discover UI it'll appear again in the Main app. This will be resolved in a follow-up branch where banner dismiss persistence will be added.
This code is non functional without the corresponding teleport updates:
Use this teleport branch which contains all of the necessary updates and hacks to QA this branch.
Start it with
TELEPORT_UNSTABLE_VC_SYNC_ON_START=yes ./build/teleport start