Feature/2070 add invitation banner #2080
Conversation
| import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; | ||
| import { faExternalLinkAlt } from "@fortawesome/free-solid-svg-icons"; | ||
|
|
||
| const SERVICE_URL = process.env.SERVICE_URL; |
There was a problem hiding this comment.
I'm not sure why linter is complaining here? This pattern is used elsewhere without complaint... and when I attempt to fix it, like so:
const {SERVICE_URL} = process.env;
SERVICE_URL becomes undefined.
There was a problem hiding this comment.
Correct, that's an unfortunate bug with webpack that eslint doesn't recognize:
prefer-destructuringconflicts with Webpack replacements ofprocess.enveslint/eslint#14918- EnvironmentPlugin: destructuring process.env doesn't work webpack/webpack#5392
You'll have to use eslint-disable-next-line
There was a problem hiding this comment.
Thanks federico. Will update soon.
| <UpdateBanner /> | ||
| <DeploymentBanner /> | ||
| <InvitationBanner /> |
There was a problem hiding this comment.
I think there should be a layout component called Banners or BannerAlert or something, that replaces these four Banners, and only one Banner will be shown at a time. As I mentioned in the first comment, will be splitting this into a separate issue.
There was a problem hiding this comment.
Thanks for creating the follow-on issue!
src/components/banner/Banner.tsx
Outdated
| variant, | ||
| children, | ||
| }) => ( | ||
| <div className={cx(styles.root, variant ? styles[variant] : styles.info)}> |
There was a problem hiding this comment.
Any tips for this kind of pattern? This is a lint warning.
There was a problem hiding this comment.
What's the warning? Object injection on styles? You can just suppress it and leave a note that the type restricts it to one of the banner variant.
The eslint rule isn't smart enough to consider the possible string values via Typescript
There was a problem hiding this comment.
NIT: I think instead of a ternary styles[variant ?? "info"] is slightly more readable
There was a problem hiding this comment.
Interesting. Using the ternary removed the object injection lint warning.
| const SERVICE_URL = process.env.SERVICE_URL; | ||
|
|
||
| const InvitationBanner: React.FunctionComponent = () => { | ||
| const { data: invitations } = useFetch<PendingInvitation[]>( |
There was a problem hiding this comment.
NIT: I think we should throw this into api.ts? For new calls I think we should be trying to use RTK Query instead of our useFetch?
Not a bit deal in this case since there's no opportunities for re-use across the components on the page since this is the only component that uses the information
| const { data: invitations } = useFetch<PendingInvitation[]>( | ||
| "/api/invitations/me/" | ||
| ); | ||
| const invitationsAvailable = useMemo(() => invitations?.length > 0, [ |
There was a problem hiding this comment.
NIT: This useMemo isn't useful here since the result is only used in one place and isn't expensive to calculate (at most there's 0-5 invitations, and length is known ahead of time anyway)
|
@mnholtz Overall this looks good. Have to run to a meeting, then will test it out quickly |
src/layout/EnvironmentBanner.tsx
Outdated
| ["development", "development"], | ||
| ["staging", "staging"], | ||
| ]); | ||
| const variantMap = new Map([ |
There was a problem hiding this comment.
Instead of casting, provide the generics directly to the constructor: new Map<string, BannerVariant>(...)
Closes #2070
This adds an Invitation Banner to the extension to notify the user when they have team invitations available, and to direct the to the Teams Page. To implement this change, a new Banner component was created to refactor repeated logic in UpdateBanner, DeploymentBanner, and EnvironmentBanner.
Additionally, I feel that only one Banner should be shown at a time (apart from the EnvironmentBanner + one other). Stacked banner notifications would risk alienating/overwhelming users, especially new users. Splitting this into a new issue - #2083, as perhaps we should rethink how users receive non-urgent/sort-of-urgent notifications.