Snackbar component

[jolierabideau]

This change is 

[rolfheij-sil]

Awesome work! Your first component! 😄

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @jolierabideau)

---

lib/papi-components/src/snackbar.component.tsx line 6 at r1 (raw file):

import './snackbar.component.css';

export type CloseReason = 'timeout' | 'clickaway' | 'escapeKeyDown';

It might be better to just copy the close reasons provided by MUI.
export type CloseReason = SnackbarCloseReason
You'd have to import SnackbarCloseReason for that to work obviously.

---

lib/papi-components/src/snackbar.component.tsx line 8 at r1 (raw file):

export type CloseReason = 'timeout' | 'clickaway' | 'escapeKeyDown';

export interface AnchorOrigin {

Here you could copy SnackbarOrigin from MUI

---

lib/papi-components/src/snackbar.component.tsx line 32 at r1 (raw file):

   * The system prop that allows defining system overrides as well as additional CSS styles.
   */
  sx?: SxProps<Theme>;

We haven't exposed the sx prop and theming for other components, so I'm not sure if we want to expose it on the snackbar

---

lib/papi-components/src/snackbar.component.tsx line 37 at r1 (raw file):

   * Additional css classes to help with unique styling of the snackbar
   */
  className?: string;

Do we need both classes and className? Don't they kinda do the same thing?

---

lib/papi-components/src/snackbar.component.tsx line 44 at r1 (raw file):

   * If true, the component is shown
   */
  open?: boolean;

This prop should be named isOpen, as per our Code style guide: https://github.com/paranext/paranext/wiki/Code-Style-Guide
One of the rules reads: Booleans: use status words like didFinish, isInitialized, or hasPolicy

Also, a default value for this prop is provided in the component, but there's no @default comment here

---

lib/papi-components/src/snackbar.component.tsx line 48 at r1 (raw file):

  /**
   * The number of milliseconds to wait before automatically calling onClose()
   * @default null

here it says null, but in the component it says 6000

---

lib/papi-components/src/snackbar.component.tsx line 55 at r1 (raw file):

   * Additional css classes to help with unique styling of the snackbar
   */
  className?: string;

Does this help with styling the bar? So shape/color stuff? And can the theming props in SnackbarContentProps only be used to style text etc inside the bar?

---

src/stories/snackbar.stories.tsx line 4 at r1 (raw file):

import { Snackbar, Button } from 'papi-components';

//

Two stray / here

---

src/stories/snackbar.stories.tsx line 17 at r1 (raw file):

    className: { control: 'text' },
    /*
     * adding actions to onClose: timeout, clickaway, or escapekeydown

I don't see any activity in my console when trying to close the snackbars. Can you explain some more?

---

src/stories/snackbar.stories.tsx line 20 at r1 (raw file):

     * they show up in actions console when they are called
     */
    onClose: { action: 'onClose' },

I'm not very familiar with Storybook yet, but I don't think this works.
Here in the argTypes section you can assign controls to your components. Most common are the boolean, text and number controls.
Since onClose is a callback function it can't be controlled by any of those.
I haven't been able to find a control type that can handle functions.
Also I'm not aware that there is an action keyword available.
Can you explain more about what you're doing here?

---

src/stories/snackbar.stories.tsx line 35 at r1 (raw file):

      classes: { root: 'papi-snackbar primary' }, // styles the root element
    },
    // className: 'primary', // styles the div surrounding snackbar

Why is this commented out?

---

src/stories/snackbar.stories.tsx line 45 at r1 (raw file):

          'This is the primary snackbar story. ' +
          'You can see the snackbar appear with a basic message and color scheme. ' +
          'If you open the actions console, you will see the snackbar calls onClose ' +

I'm not seeing the behavior you describe here. Also onClose isn't implemented here, so I don't see how it could produce the behavior you describe. Am I missing something?

[rolfheij-sil]

Also a more general question: In the main Storybook view for the Snackbar (the view that shows an overview of all stories for the Snackbar), the entire stack of snackbars is rendered in the bottom-left corner of the window. Is it possible to have each snackbar show up in it's own little render box?

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @jolierabideau)

[jolierabideau]

Thank you for your review! As far as your general question, I am not sure I entirely understand. Are you asking if it is possible to have the snackbars appear fully in the windows shown, instead of only the corner? I attached a photo of what I see in the main Storybook view for the Snackbar.

Reviewable status: 4 of 6 files reviewed, 12 unresolved discussions (waiting on @rolfheij-sil)

---

lib/papi-components/src/snackbar.component.tsx line 6 at r1 (raw file):

Previously, rolfheij-sil wrote…

It might be better to just copy the close reasons provided by MUI.
export type CloseReason = SnackbarCloseReason
You'd have to import SnackbarCloseReason for that to work obviously.

Done.

---

lib/papi-components/src/snackbar.component.tsx line 8 at r1 (raw file):

Previously, rolfheij-sil wrote…

Here you could copy SnackbarOrigin from MUI

Done.

---

lib/papi-components/src/snackbar.component.tsx line 32 at r1 (raw file):

Previously, rolfheij-sil wrote…

We haven't exposed the sx prop and theming for other components, so I'm not sure if we want to expose it on the snackbar

Okay, I wasn't entirely sure about that one. I removed it.

---

lib/papi-components/src/snackbar.component.tsx line 37 at r1 (raw file):

Previously, rolfheij-sil wrote…

Do we need both classes and className? Don't they kinda do the same thing?

I did some more research, and yes I think they do basically the same thing. It looks like the classes attribute might give you more access to style child elements, but I think className will give what we are looking for. I removed the classes attribute.

---

lib/papi-components/src/snackbar.component.tsx line 44 at r1 (raw file):

Previously, rolfheij-sil wrote…

This prop should be named isOpen, as per our Code style guide: https://github.com/paranext/paranext/wiki/Code-Style-Guide
One of the rules reads: Booleans: use status words like didFinish, isInitialized, or hasPolicy

Also, a default value for this prop is provided in the component, but there's no @default comment here

Done.

---

lib/papi-components/src/snackbar.component.tsx line 48 at r1 (raw file):

Previously, rolfheij-sil wrote…

here it says null, but in the component it says 6000

From what I saw, the API says that the default value is null, but it is recommended that if autoHideDuration is enabled you give users enough time, and so they use 6000 milliseconds as the default for number.

---

lib/papi-components/src/snackbar.component.tsx line 55 at r1 (raw file):

Previously, rolfheij-sil wrote…

Does this help with styling the bar? So shape/color stuff? And can the theming props in SnackbarContentProps only be used to style text etc inside the bar?

The SnackbarProps className applies style to the <div> surrounding the snackbar. The SnackbarContentProps className applies style to the bar itself.

---

src/stories/snackbar.stories.tsx line 4 at r1 (raw file):

Previously, rolfheij-sil wrote…

Two stray / here

Done.

---

src/stories/snackbar.stories.tsx line 17 at r1 (raw file):

Previously, rolfheij-sil wrote…

I don't see any activity in my console when trying to close the snackbars. Can you explain some more?

I'm not sure if you were looking here- but it is not the Javascript console, it is within Storybook itself. If you click the settings icon and have it "Show addons", then you should see a tab for actions. Then, if you hit the escape key, wait for timeout, or click the screen you will see the onClose action. The clickaway refers to a click outside of the component, that is why it doesn't work if you click the component itself. I attached a photo showing what I see.

---

src/stories/snackbar.stories.tsx line 20 at r1 (raw file):

Previously, rolfheij-sil wrote…

I'm not very familiar with Storybook yet, but I don't think this works.
Here in the argTypes section you can assign controls to your components. Most common are the boolean, text and number controls.
Since onClose is a callback function it can't be controlled by any of those.
I haven't been able to find a control type that can handle functions.
Also I'm not aware that there is an action keyword available.
Can you explain more about what you're doing here?

This is one of the sites I used when I was looking into actions: https://storybook.js.org/docs/react/essentials/actions.

I have not found a control type that handles functions either. The action keyword here is telling Storybook that this argument is not something that can be controlled with the values you listed, but it is an action. Then I believe it creates the argument as a callback so that it can be called, and when it is called it will show up in the Actions console that I included in a photo in the previous comment.

---

src/stories/snackbar.stories.tsx line 35 at r1 (raw file):

Previously, rolfheij-sil wrote…

Why is this commented out?

The className prop that is a part of the general SnackbarProps (and not a part of the ContentProps) applies to the <div> element surrounding the snackbar, not the bar itself. I did not have any style classes focused on the <div>, but left it to demonstrate the difference between styling the <div> and the bar itself.

---

src/stories/snackbar.stories.tsx line 45 at r1 (raw file):

Previously, rolfheij-sil wrote…

I'm not seeing the behavior you describe here. Also onClose isn't implemented here, so I don't see how it could produce the behavior you describe. Am I missing something?

You are right the onClose is not implemented here, but by telling Storybook that it is an action it can pretend that it is closing by showing us in the Actions console that a callback was fired. The snackbar itself will not actually close in Storybook. I believe it is just a way to showcase what kinds of things would call the onClose and when. I hope my comments/screen shots helped a little, if not I would love to share my screen and talk through some of this with you. If it would be better for me to remove the action until we all have a better understanding of it, I can do that as well.

[rolfheij-sil]

Interesting! This is what it looks like on my end
Chrome Version 114.0.5735.134 on Windows 11.
You're on Safari/MacOS, right?

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jolierabideau)

---

lib/papi-components/src/snackbar.component.tsx line 48 at r1 (raw file):

Previously, jolierabideau wrote…

From what I saw, the API says that the default value is null, but it is recommended that if autoHideDuration is enabled you give users enough time, and so they use 6000 milliseconds as the default for number.

Ah yes, I get the confusion. The default value specified by MUI is null, but on line 85 of this file you've overridden that default to be 6000

---

lib/papi-components/src/snackbar.component.tsx line 55 at r1 (raw file):

Previously, jolierabideau wrote…

The SnackbarProps className applies style to the <div> surrounding the snackbar. The SnackbarContentProps className applies style to the bar itself.

Thanks for clarifying

---

src/stories/snackbar.stories.tsx line 17 at r1 (raw file):

Previously, jolierabideau wrote…

I'm not sure if you were looking here- but it is not the Javascript console, it is within Storybook itself. If you click the settings icon and have it "Show addons", then you should see a tab for actions. Then, if you hit the escape key, wait for timeout, or click the screen you will see the onClose action. The clickaway refers to a click outside of the component, that is why it doesn't work if you click the component itself. I attached a photo showing what I see.

Interesting. It's not showing anything on my end. Let's have a look at it later today

---

src/stories/snackbar.stories.tsx line 20 at r1 (raw file):

Previously, jolierabideau wrote…

This is one of the sites I used when I was looking into actions: https://storybook.js.org/docs/react/essentials/actions.

I have not found a control type that handles functions either. The action keyword here is telling Storybook that this argument is not something that can be controlled with the values you listed, but it is an action. Then I believe it creates the argument as a callback so that it can be called, and when it is called it will show up in the Actions console that I included in a photo in the previous comment.

Cool! Seems like a really nice feature.

---

src/stories/snackbar.stories.tsx line 35 at r1 (raw file):

Previously, jolierabideau wrote…

The className prop that is a part of the general SnackbarProps (and not a part of the ContentProps) applies to the <div> element surrounding the snackbar, not the bar itself. I did not have any style classes focused on the <div>, but left it to demonstrate the difference between styling the <div> and the bar itself.

Ah, that makes sense. Perhaps a good idea to add some styling to the <div> too? Seems like a useful thing to show here

---

src/stories/snackbar.stories.tsx line 45 at r1 (raw file):

Previously, jolierabideau wrote…

You are right the onClose is not implemented here, but by telling Storybook that it is an action it can pretend that it is closing by showing us in the Actions console that a callback was fired. The snackbar itself will not actually close in Storybook. I believe it is just a way to showcase what kinds of things would call the onClose and when. I hope my comments/screen shots helped a little, if not I would love to share my screen and talk through some of this with you. If it would be better for me to remove the action until we all have a better understanding of it, I can do that as well.

Yep, let's talk this through! Seems like you did a great job, and that there might be some cross-platform weirdness going on

[jolierabideau]

That is interesting. Yes, Safari version 16.5 (18615.2.9.11.4) on MacOS 13.4. I was running Storybook locally.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @rolfheij-sil)

---

lib/papi-components/src/snackbar.component.tsx line 48 at r1 (raw file):

Previously, rolfheij-sil wrote…

Ah yes, I get the confusion. The default value specified by MUI is null, but on line 85 of this file you've overridden that default to be 6000

They are both null now, I missed it with my last commit.

---

lib/papi-components/src/snackbar.component.tsx line 55 at r1 (raw file):

Previously, rolfheij-sil wrote…

Thanks for clarifying

Done.

---

src/stories/snackbar.stories.tsx line 17 at r1 (raw file):

Previously, rolfheij-sil wrote…

Interesting. It's not showing anything on my end. Let's have a look at it later today

Sounds good.

---

src/stories/snackbar.stories.tsx line 20 at r1 (raw file):

Previously, rolfheij-sil wrote…

Cool! Seems like a really nice feature.

It is! There are still some things I can learn about it as well.

---

src/stories/snackbar.stories.tsx line 35 at r1 (raw file):

Previously, rolfheij-sil wrote…

Ah, that makes sense. Perhaps a good idea to add some styling to the <div> too? Seems like a useful thing to show here

I agree, working on this.

---

src/stories/snackbar.stories.tsx line 45 at r1 (raw file):

Previously, rolfheij-sil wrote…

Yep, let's talk this through! Seems like you did a great job, and that there might be some cross-platform weirdness going on

Done.

[rolfheij-sil]

As we saw during our meeting yesterday, it works correctly in Firefox on Windows 11. Not sure why Chrome is handling things differently, but probably not worth spending time to figure that out right now

Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jolierabideau)

---

src/stories/snackbar.stories.tsx line 51 at r3 (raw file):

    ContentProps: {
      message: 'This snackbar will timeout',
      action: '',

What does it mean to define the action as '' here?

[jolierabideau]

@rolfheij-sil I am still working on connecting the papi Button to the Snackbar, like we talked about yesterday. So if we want to wait to do anything with the PR until those changes are added, I can hopefully finish that today.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

---

src/stories/snackbar.stories.tsx line 51 at r3 (raw file):

Previously, rolfheij-sil wrote…

What does it mean to define the action as '' here?

This is a mistake on my part, I meant to remove this line. This action refers to the action attribute in SnackbarContentProps, not the action addon for Storybook. Defining it like this just means it is empty, but I am removing the line. Sorry about that, thank you!

[rolfheij-sil]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jolierabideau)