Update the snackbar notification design

[bl-nero]

Figma

Contributes to #45977
Requires #45973 (because I use the exported Action interface here).
Followed up by https://github.com/gravitational/teleport.e/pull/4972

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[bl-nero]

Note: I'm not closing the issue just yet with this PR; there's an opportuinity to (1) improve the desktop viewer that doesn't adhere to our UI patterns here, and (2) make it generally much easier to use the notifications in web UI by moving the notification service from Teleterm to shared components. I'll address this after I deal with more important work from the UX design bucket.

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[bl-nero]

Manual testing: tested the notifications on account settings screen, Teleport Connect app, and desktop session.

[bl-nero]

Addressed Rafał's feedback and corrected the plain string case (there was a separate design for this case in the Figma deck that I didn't catch).

[ravicious]

Looks good overall, I submitted a couple of minor suggestions, with the one about putting single strings into title rather than description being almost major.

[ravicious]

We used to enforce that at least title is always present. This type here makes it legal to create a notification with pretty an empty content because every field is optional.

What if we still enforced some minimum required fields?

export type NotificationItemObjectContent = (
  | { title: string; subtitle?: string; description?: string }
  | { description: string; title?: undefined; subtitle?: undefined }
) & {
  list?: string[];
  icon?: React.ComponentType<IconProps>;
  action?: Action;
};

This type forces you to always provide at least title and other fields or just description. I think that's the intention behind toObjectContent. The type also does not allow you to provide description and subtitle but no title, which is what we want to avoid, I think.

If we go with this type, there's one example in the story which actually breaks it, the notification with a list and nothing else.

[bl-nero]

I agree that we might make it more strict, but think that this level of complexity is a bit of an overkill here. I'm a bit afraid that if we make it this complex, it's gonna bubble up the type hierarchy in unpredictable ways and cause errors with very confusing messages. For now, I'm fine with a risk of someone creating an empty notification. Am I missing something obvious here?

[ravicious]

It's true that the error messages generated by this type might end up being confusing. When I'm considering this tradeoff myself, I usually brush off the problem with error messages by believing that TypeScript will only improve its error messages over time [1], whereas we can enforce more safety through types right now.

When I encounter a confusing type error, I usually try to look up its source, which in this case would guide me towards a fix. The error message gets kind of better when we give names to individual types, but it's still far from perfect.

Types with names

type TitleWithOptionalSubtitleAndDescription = {
  title: string;
  subtitle?: string;
  description?: string;
};

type DescriptionOnly = {
  description: string;
  title?: undefined;
  subtitle?: undefined;
};

export type NotificationItemObjectContent = (
  | TitleWithOptionalSubtitleAndDescription
  | DescriptionOnly
) & {
  list?: string[];
  icon?: React.ComponentType<IconProps>;
  action?: Action;
};

        <Notification
          …
            content: {
              subtitle: 'siemka',
            },
        />


web/packages/shared/components/Notification/Notification.story.tsx:69:13 - error TS2322: Type '{ subtitle: string; }' is not assignable to type 'NotificationItemContent'.
  Type '{ subtitle: string; }' is not assignable to type 'TitleWithOptionalSubtitleAndDescription & { list?: string[]; icon?: ComponentType<IconProps>; action?: Action; }'.
    Property 'title' is missing in type '{ subtitle: string; }' but required in type 'TitleWithOptionalSubtitleAndDescription'.

69             content: {
               ~~~~~~~

  web/packages/shared/components/Notification/types.ts:39:3
    39   title: string;
         ~~~~~
    'title' is declared here.
  web/packages/shared/components/Notification/types.ts:31:3
    31   content: NotificationItemContent;
         ~~~~~~~
    The expected type comes from property 'content' which is declared here on type 'NotificationItem'

It's fine if you want to go for optional types though. If we find that the lack of them poses a real problem, we can always add them in the future, which shouldn't require big changes.

[1]: Because in my mind it's their duty to catch up to where Elm and Rust were years ago. Which might not actually be true, hence "believing". 😶

[bl-nero]

@ravicious Changed the string-only default to title-only, got rid of the unnecessary space, but pushing back on the type changes. PTAL.

[ravicious]

It's true that the error messages generated by this type might end up being confusing. When I'm considering this tradeoff myself, I usually brush off the problem with error messages by believing that TypeScript will only improve its error messages over time [1], whereas we can enforce more safety through types right now.

When I encounter a confusing type error, I usually try to look up its source, which in this case would guide me towards a fix. The error message gets kind of better when we give names to individual types, but it's still far from perfect.

Types with names

type TitleWithOptionalSubtitleAndDescription = {
  title: string;
  subtitle?: string;
  description?: string;
};

type DescriptionOnly = {
  description: string;
  title?: undefined;
  subtitle?: undefined;
};

export type NotificationItemObjectContent = (
  | TitleWithOptionalSubtitleAndDescription
  | DescriptionOnly
) & {
  list?: string[];
  icon?: React.ComponentType<IconProps>;
  action?: Action;
};

        <Notification
          …
            content: {
              subtitle: 'siemka',
            },
        />


web/packages/shared/components/Notification/Notification.story.tsx:69:13 - error TS2322: Type '{ subtitle: string; }' is not assignable to type 'NotificationItemContent'.
  Type '{ subtitle: string; }' is not assignable to type 'TitleWithOptionalSubtitleAndDescription & { list?: string[]; icon?: ComponentType<IconProps>; action?: Action; }'.
    Property 'title' is missing in type '{ subtitle: string; }' but required in type 'TitleWithOptionalSubtitleAndDescription'.

69             content: {
               ~~~~~~~

  web/packages/shared/components/Notification/types.ts:39:3
    39   title: string;
         ~~~~~
    'title' is declared here.
  web/packages/shared/components/Notification/types.ts:31:3
    31   content: NotificationItemContent;
         ~~~~~~~
    The expected type comes from property 'content' which is declared here on type 'NotificationItem'

It's fine if you want to go for optional types though. If we find that the lack of them poses a real problem, we can always add them in the future, which shouldn't require big changes.

[1]: Because in my mind it's their duty to catch up to where Elm and Rust were years ago. Which might not actually be true, hence "believing". 😶

[bl-nero]

@ravicious Thanks for the type naming trick, I'll keep it in mind the next time when I'll need to get creative with the type system. :)

(Oh, yes, if we had a Rust-level error experience, life would be so much easier.)

[public-teleport-github-review-bot]

@bl-nero - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

[bl-nero]

Sorry for the mess, I accidentally merged master into it before merging the dependency.

[public-teleport-github-review-bot]

@bl-nero See the table below for backport results.

Branch	Result
branch/v16	Failed