material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[Joy] Add `Alert` component

Open hbjORbj opened this issue 3 years ago • 2 comments

Done:

  • Initial implementation
  • Initial demos
  • Props: color, variant, onClose, closeText, etc; To be more discussed with Jun and Danilo

To do:

  • Add tests
  • Improve demos/implementation with Jun and Danilo's feedback

Preview: https://deploy-preview-33859--material-ui.netlify.app/joy-ui/react-alert/

hbjORbj avatar Aug 08 '22 12:08 hbjORbj

Details of bundle changes

@mui/joy: parsed: +1.13% , gzip: +0.61%

Generated by :no_entry_sign: dangerJS against 54f150b4e3e776c7dac8902257e3c4685f0cf06a

mui-bot avatar Aug 08 '22 13:08 mui-bot

@danilo-leal

It looks much much nicer! I see that you've changed the default variant to soft in the playground. I will do the same in the implementation.

Lastly, a quick question: will we support something similar to the AlertTitle component?

I will let @siriwatknp answer this.

hbjORbj avatar Aug 09 '22 06:08 hbjORbj

@hbjORbj Thanks for the hard work

Here are things I think can be improved:

HTML structure

Let's keep the rendered HTML simple. I am not sure why Material UI produces many div but I think this is simpler:

<Alert>...</Alert>

// The result should be just 1 div
<div role="alert" class="JoyAlert-root">...</div>

// not this
<div role="alert" class="JoyAlert-root">
  <div class="JoyAlert-message">...</div>
</div>

Size

It should have size prop which controls the icon and text font size. (similar to other components, e.g. Button)

Decorator

The alert should accept startDecorator and endDecorator similar to other components, e.g. Input.

You can remove the icon and action props.

The usage should be like this:

<Alert startDecorator={<Icon />} endDecorator={<IconButton><Close /></IconButton>}>
  <Typography fontWeight="lg" mb={1}>Alert title</Typography>
  <Typography level="body2">Description</Typography>
</Alert>

CSS variables

It should have:

  • --Alert-radius
  • --Alert-gap
  • --Alert-padding

You can take a look at the Input component. It is quite similar.

AlertTitle

I don't see a need for the AlertTitle at this point. Let's try to create demos using Typography first.

Common examples

Can you add some of these to the common examples section:

  • https://www.pinterest.com/pin/492649947198210/
  • https://www.pinterest.com/pin/376191375137451403/
  • https://www.pinterest.com/pin/223280094015936511/

siriwatknp avatar Aug 16 '22 07:08 siriwatknp

@hbjORbj The implementation looks good to me. What's left are:

  • component tests (also add the testCustomVariant: true to the describeConformance, it ensures that the component does not break when a custom variant is provided.)
  • add the AlertOwnerState to components.d.ts
  • add to extendTheme.spec.ts

siriwatknp avatar Aug 29 '22 02:08 siriwatknp

Done:

  • ✅ component tests (also add the testCustomVariant: true to the describeConformance, it ensures that the component does not break when a custom variant is provided.)
  • ✅ add the AlertOwnerState to components.d.ts
  • ✅ add to extendTheme.spec.ts

hbjORbj avatar Aug 29 '22 13:08 hbjORbj