deprecate / metal-clay-components

10 stars 14 forks source link

ClayAlert notifications type #136

Closed matuzalemsteles closed 6 years ago

matuzalemsteles commented 6 years ago

Just continuing the #134 dialog so that we can get to a ClayAlert API definition.

The way we treat the ClayAlert API as just an alert, prevents us from adding the types of notifications.

Both wrap up several alerts so they can be stacked, for example:

<div class="alert-notifications alert-notifications-fixed">
    <div class="alert ..."></div>
    <div class="alert ..."></div>
    <div class="alert ..."></div>
    <div class="alert ..."></div>
</div>

That way we can not support ClayAlert this option. I want to leave my vision on this so that we can arrive at a better experience.

Proposal

Option 1

We can refactor the entire ClayAlert API so that devs can pass multiple alerts on the component.

Example:

{
    alerts: [
        {
            autoClose: true,
            message: '',
            style: 'success',
            title: 'Success',
            type: 'fluid',
        },
        {
            message: '',
            style: 'warning',
            title: 'Warning',
            type: 'fluid',
        }
    ],
    spritemap: spritemap,
}

The bad thing about this approach is that we give a certain "freedom" for devs to be able to pass several nested alerts and we can not control it, I do not feel secure about it.

In use cases where you only want to pass only an information alert the use of ClayAlert may become complex in a taglib JSP for example what was meant to be simple.

Example:

{
    alerts: [
        {
            autoClose: true,
            message: '',
            style: 'success',
            title: 'Success',
            type: 'fluid',
            alerts: [
                {
                    message: '',
                    style: 'warning',
                    title: 'Warning',
                    type: 'fluid',
                }
            ]
        }
    ],
    spritemap: spritemap,
}

Option 2

We can maintain the current ClayAlert API and treat notification types as a second component, making it optional, and making ClayAlert simple to use in cases where the dev only wants to display an alert message.

Example of the use in soy:

{call ClayAlertTypeNotification.render}
    {param content kind="html"}
        {call ClayAlert.render}
            {param message: '' /}
            {param title: 'Info' /}
        {/call}
        {call ClayAlert.render}
            {param message: '' /}
            {param title: 'Info' /}
        {/call}
        {call ClayAlert.render}
            {param message: '' /}
            {param title: 'Info' /}
        {/call}
    {/param}

    {param type: 'fixed' /}
{/call}

Unlike the second option the use of ClayAlert becomes simpler in a taglib jsp when dev just wants to display an alert message.

cc @carloslancha, @jbalsas

carloslancha commented 6 years ago

A bunch of toast alerts just need to be inside a container with classes alert-notifications alert-notifications-fixed (absolute notifications are not defined in Lexicon). Do we need a component for that?

We will have to deal with this when integrating in portal, but for now I'd stay where we are. If somebody wants to position toast alerts the need to place'em inside a container with those specific classes.

matuzalemsteles commented 6 years ago

Hey @carloslancha, I agree with you, I think we can close this and if anyone has any different opinion we will review.