Feedback Indicator

[drakonux]

Feedback Indicator

Due to LEXI-765 request by team-tango, we would like to add a new component to give feedback. It's basically a non-boxed alert that has no interaction. However, a link can be placed within the message.

There are 4 types of feedback indicators as we have for alerts.

And as we talked offline instead of the initial name of "alert indicator" use "feedback indicator".

The definition is published on the lexicon site and you can access LEXI-916 where we worked. There is a link to the definition document.

If you need additional work or definition please don't hesitate to request it.

[dgarciasarai]

cc: @susanavr

[pat270]

@drakonux Eventually we could replace alert-indicator and form-feedback-indicator with this feedback-indicator but we should keep everything for now so we don't break anything.

[drakonux]

I'm not pretty sure the differences between alert-indicator and form-feedback-indicator or if it makes sense to have two different names for the same usage.

We want to bring consistency in the naming and usage because it's just an icon + text (optional)

How could we prevent a break in the future? I mean, we don't want to change the name again. The pattern is straightforward and probably used in some scenarios with actions (button link, action button), but those buttons are not part of the component or should be part of it?

[pat270]

The problem with replacing it now is that all our code in Clay and Portal are using the old CSS classes. We would definitely break things during this stabilization period. The patterns alert-indicator and form-feedback-indicator have been in place since 7.1, swapping them out now would cause upgrade issues for anyone relying on those patterns coming to 7.4 (inconsistency for our upgrading customers and component teams).

I think we need to be more careful with these kinds of changes in the name of consistency because it can actually introduce inconsistency (e.g., changing times => times-small icon). It would be nice to have a time and place to make breaking changes so we can better document and provide a heads up to those it might affect.

In my opinion icon + text pattern name should be different from feedback. icon + text are more like an atom? molecule? and feedback is more like a molecule? organism?. From my understanding, feedback provides information on the state of the UI relative to a user's input. It doesn't seem to fit with the icon + text pattern inside a button. They should be two separate things.

Clay does have an icon + text pattern implementation already. You can see it at https://clayui.com/docs/css/utilities/inline-item.html.

[drakonux]

Just to be clear. We should not break things if we are not sure this is gonna bring something useful for developers using Clay.

ps. Talking offline with @pat270 I think we should reconsider this new component and try to add the info type to form-indicator sounds like a more feasible solution. Besides, I would like to talk with @dgarciasarai to understand if we are missing something here.

[pat270]

I'll upload some demos you can take a look at using components we already have.

[dgarciasarai]

Hey @pat270 @drakonux, On my day to day, I use Lexicon and Clay websites to know which components are already available to use when coding. In my case, we needed a simple indicator, we saw it in Lexicon's website but I didn't find it in the same component in Clay's website (Alert). In my use case, I'm not using a form, so for me use that component seems a little bit weird. My humble proposal is to have another variant for the ClayAlert component called for example indicator that keeps the alert as simple as possible (such as Lexicon's documentation): without border, without background and even without padding and margin and let the user add it in case s/he will need it. Something like:

<ClayAlert displayType="warning" dismissible={true} title="Warning" variant="indicator">
    This is a warning alert indicator
</ClayAlert>

I think this option won't break things but I'm not an expert in this area. What do you think? Let's take the better decision! Thanks! 😄

[pat270]

@dgarciasarai This would work. The way you would use it for a single indicator would be:

<ClayAlert className="alert-feedback alert-inline" displayType="warning" spritemap={spritemap}>
</ClayAlert>

I need to add those modifiers to Clay CSS and we need to make some adjustments to ClayAlert to not output some markup if a title isn't declared.

[pat270]

I have a few questions about how feedback should render. Should it: 1) Everything in a line? 2) Text aligns right of icon and text aligns left of dismiss button? 3) Dismiss button always top right?

[jbalsas]

Hey @pat270, I'm not sure this is an approach we want to take. Placing this inside ClayAlert might make sense, but I don't think we can have a basic component that needs to be specified always passing down className="alert-feedback alert-inline".

Also, the fact that

I need to add those modifiers to Clay CSS and we need to make some adjustments to ClayAlert to not output some markup if a title isn't declared.

Should be a bit of a red flag that we're starting to use a component for something that wasn't intended for. Remember our old tags, they're riddled with branches and special cases because we try to put too much in the same place and end up completely blurring any separation between components.

We need to think this through and take a bit of time to get it right

[kresimir-coko]

Reading through this conversation it seems to me like we very quickly jumped from "make a new component" to "adapt ClayAlert to fit our needs". This looks to me like a very easy decision to make a new component, here's why:

• Doesn't introduce breaking changes
• Doesn't inflate ClayAlert with unexpected use cases
• Is easy to develop, test, and introduce to DXP
• Is independent on other Clay components and thus can be imported granularly

The only downside is that it adds another component to our library, but is that really so bad (and isn't that the point of Clay - to have varied components for our devs)? I'd rather have 100 small components than 20 components with branching implementation.

[jbalsas]

The only downside is that it adds another component to our library, but is that really so bad (and isn't that the point of Clay - to have varied components for our devs)? I'd rather have 100 small components than 20 components with branching implementation.

brainstorm of @clayui/core package #3966 might disagree with this ;)

[ambrinchaudhary]

Here I agree with @kresimir-coko, it seems that a new simple component would fit better for the reasons he states. Also, take in mind that this is not really an alert, that "are used to capture the attention of the user in an intrusive way"... it is just feedback (FeedbackIndicator fits good for me), and does not need to be in a form, or in a specific position in the screen (alerts must be on top of the form, or in a toast...)

[wincent]

The only downside is that it adds another component to our library, but is that really so bad (and isn't that the point of Clay - to have varied components for our devs)? I'd rather have 100 small components than 20 components with branching implementation.

brainstorm of @clayui/core package #3966 might disagree with this ;)

Although that one is about how many packages we have (tl;dr: too many, modularizing at the package level brings lots of overhead), not how many components we have (this is the layer at which the lots-of-small-components vs fewer-complex-components trade-off gets explored).

[jbalsas]

Although that one is about how many packages we have (tl;dr: too many, modularizing at the package level brings lots of overhead), not how many components we have (this is the layer at which the lots-of-small-components vs fewer-complex-components trade-off gets explored).

True, true, I collapsed both, assuming new component implied also new package, but that does not necessarily have to be the case.

[kresimir-coko]

Yea, I think that something that you would probably tell me in an alternate universe @jbalsas would be that more components !== more packages, it's how we package those components is the issue we need to fix ❤️

[drakonux]

Before answering your questions @pat270 , I think Lexicon has to document better its requests, I thought this one was easy and I was confident with the work done months ago. But here we miss that if it's a variant (as we thought it was) there are many features that we didn't describe (title, autoclose, dismissible, etc).

1. Everything in a line?

2. Text aligns right of icon and text aligns left of dismiss button?

We would like to follow the same alignment we planned for alerts. See the previous image.

3. Dismiss button always top right?

If it's dismissible, yes.

About creating a new component or variant. Your choice. That's why we asked you offline because we just want to have the documentation aligned with Clay.

If you want to combine this request with other similar components like form-indicator, we think is great too. But the context of this request was because alerts are too big and captive too much user's attention to be used in a sidebar.

In our opinion, Clay must understand how developers will use and look for this so as @dgarciasarai said she expects to have it under alerts and the option of form-indicator where there is no form, it's weird indeed. However, this is a technical decision so... up to you.

[pat270]

Clay already has the component needed to create icon + text + spacing + colors (https://clayui.com/docs/css/utilities/inline-item.html with text-{color}). We can package this in the new component or reimplement it like I did in original pr (https://github.com/liferay/clay/pull/3979).

Pros:

• Clean namespace easy to target across Portal
• Doesn't inflate ClayAlert with unexpected use cases
• Is independent on other Clay components and thus can be imported granularly
• We could eventually replace alert-indicator and form-feedback-indicator patterns with this? If we are creating a stand alone React component I don't have much faith in being able to replace the other indicators (inflexible markup reasons).
• Don't have to always pass down className="alert-feedback alert-inline" or some variant of that

Cons:

• Dismissible and auto close will have to be rewritten or we extract the functionality from Alert to make it reusable (unless it's already like that, I didn't check)
• Depending on Lexicon definition we will need to duplicate the markup pattern in Alert
• Replacing alert-indicator and form-feedback-indicator patterns would break the markup we have been using since 7.1

From @drakonux's screenshot, I think we should piggy back of alerts since the structure is already there, just need to remove padding, border, and background similar to what is done with btn-outline-primary btn-outline-borderless.

For the patterns below (Total Views, Total Reads, Views, Reads): Lumping them with alert doesn't seem like a good idea. These look like a good use case for utility classes (inline-item, text-secondary, text-dark, text-warning or https://clayui.com/docs/components/link/markup.html).

[wincent]

This is currently a "nice to have" for Tango for 7.4, so urgency is not great, but it would be good to have an estimate for when we think we'll drive this one to a conclusion. I think somebody just needs to take a decision, weighing up the pros and cons listed above. I notice that @brunofernandezg moved this from the March 24th milestone to the April 7th one. Does that still sound realistic? (It may not be.) Just so that any Tango folks watching this can have visibility into the status.

[kresimir-coko]

@pat270 These look like a good use case for utility classes (inline-item, text-secondary, text-dark, text-warning

Let's say we decide to take this route, what does it mean exactly? Does it mean that we will just provide the classes and let devs in DXP use those classes to alter their Alerts to look like Feedback Indicators? If it does, then we need to have a better way of documenting utility classes.

Depending on Lexicon definition we will need to duplicate the markup pattern in Alert

HTML is super fast, is it not, so it shouldn't be that big of a deal to have 2 components on the same page with just slight alterations to their Markup.

Dismissible and auto close will have to be rewritten or we extract the functionality from Alert to make it reusable (unless it's already like that, I didn't check)

I don't know either, from the top of my head, but we do have the shared folder/package, don't we? Couldn't that be used to contain that functionality, and then in the future, if we ever need a 3rd component that needs that functionality, all of that work will be already done. So it's kind of a question if we aim for long term or short term. I'd say long term, because it's not something that's incredibly urgent.

It might come across that I'm fairly strongly fighting for a new component here, but I'm just trying to get some information as well as try to ask the important questions.

[matuzalemsteles]

Well, looking at this now, I think we need a direction but I still have a question to see which direction we are going to go. Analyzing the previous comments I realized the following.

• In case the Implementation Feedback Indicator follows the same behaviors as an Alert, in the case of just removing the background and adding the color, we must follow this implementation as an displayType of an alert. The spec does not seem to be the case. cc @drakonux
• The Feedback Indicator request seems to impose that the icon always appears and in some cases just the icon, so this seems different from the expected behavior of a Feedback Indicator of the forms, although they appear to be the same. I remember that @bryceosterhaus came to comment on that too.

In summary, we must go with a new independent Feedback Indicator component of the form (although they are the same, maybe we can only use the same markup under the hood in the implementation but we can do that at another time) but it is worth knowing if it will have the same resources than the Alert, this can change the direction of that implementation. This component can live within the Alert package as it is a variation of the Alert with less attention.

@pat270 I like the idea of using only the utilities mainly if this implementation does not contain all the behaviors of the traditional Alert, so we can implement this component using only the utils. This is good because we don't need to add more css to support just this component and our utils can do the same.

For now, we can only keep the Feedback Indicator of the form as it is, because its implementation is still attached to the form, it depends on classes like has-* in the parent element to define the color. @pat270 maybe we can try to use the same markup for both cases but I think that would just be worth a change like this in a v4 version.

[wincent]

Folks from Tango were asking about this again yesterday, so it would be good to drive this to a conclusion.

I also got some private feedback (you might say I got a feedback indicator... ha!) that irrespective of implementation details, from the perspective of design and developers who will use this, it is conceptually an alert without a box, which is what @drakonux said in the original summary:

it's basically a non-boxed alert that has no interaction.

So using any "form" related terminology (eg. "form indicator") may be confusing. Again, regardless of implementation, at a conceptual level people are going to think of this as a variant of an alert.

I don't want to call the shots because I'm not the one implementing this. Is it going to be you who implements it @matuzalemsteles? (Not clear on how much time you have for Clay with the reprioritization that has gone on in Brazil recently.) Can you take the lead on this?

[matuzalemsteles]

@wincent yeah, I also see this more as an alert variant if we follow all alert behaviors, so I would just see it as a new property within the alert. Well, if that's the case, I think we should go in that direction instead of creating a new markup and a new component for that, @ pat270 you who was thinking in another direction, what do you think?

So using any "form" related terminology (eg. "form indicator") may be confusing. Again, regardless of implementation, at a conceptual level people are going to think of this as a variant of an alert.

I think we call Form Indicator because there is a similar implementation for Form more for other purposes, so we can leave it as it is there and implement it separately near the Alert to avoid confusion.

I don't want to call the shots because I'm not the one implementing this. Is it going to be you who implements it @matuzalemsteles? (Not clear on how much time you have for Clay with the reprioritization that has gone on in Brazil recently.) Can you take the lead on this?

Yeah, I can take care of implementing this, I'm working part-time now, so I'm going to try to get more involved here. But I will be back 100% within two weeks

[brunofernandezg]

Hi, Have we made a decision here? I think the mention to @pat270 failed in Matu's comment.

Thanks.

[matuzalemsteles]

Oh, thank you @brunofernandezg! I ended up not realizing it....

I think we just need to know @drakonux or @hold-shift and @pat270 before starting work.

[pat270]

Going with alert is good with me, it requires the least amount of work (for the group not for me). I think @jbalsas was worried about trying to fill too many functions with alert.

[jbalsas]

Going with alert is good with me, it requires the least amount of work (for the group not for me). I think @jbalsas was worried about trying to fill too many functions with alert.

I'll let you two figure it out. It just brings me reminiscences from the past. Just look around any component like ui:icon that might as well output an icon, a link, a button, a dropdpown... 🤷

So, I'd be more in favour of: <ClayAlert.Indicator> than <ClayAlert variant="indicator"> in that sense, but I'll step aside now and let you handle it 🤗

[matuzalemsteles]

@jbalsas hahah I agree, I also do not want the component to be bloated. My question here is to know if this implementation is very nested with the same features as the Alert, resources we have and just add a new variation to the existing API, we do the same for stripe, so I think that would be much more consistent.

[jbalsas]

Sounds good to me! Up to you guys! 👊

El El lun, 12 abr 2021 a las 18:40, Matuzalém Teles < @.***> escribió:

@jbalsas https://github.com/jbalsas hahah I agree, I also do not want the component to be bloated. My question here is to know if this implementation is very nested with the same features as the Alert, resources we have and just add a new variation to the existing API, we do the same for stripe, so I think that would be much more consistent.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/liferay/clay/issues/3968#issuecomment-817999448, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG46LTNQL6AMQMRNGUEFPLTIMV7DANCNFSM4YQ4EALQ .

-- Chema Balsas Software Developer Liferay España y Portugal / Liferay Spain and Portugal Tel. +34 917336343

http://liferay.com/ Enterprise. Open Source. For Life Síguenos en http://www.facebook.com/pages/Liferay/45119213107 http://www.facebook.com/pages/Liferay/45119213107 https://twitter.com/liferay_es http://www.liferay.com/about-us/news

Visit Us: www.liferay.com | Like Us: facebook.com/liferay | Follow Us: twitter.com/liferay_es

Aviso de Confidencialidad: Este mensaje de correo electrónico, incluidos los archivos adjuntos, enviados en nombre de Liferay SL es para el uso exclusivo del destinatario (s) previsto y puede contener información confidencial y privilegiada. Se prohíbe cualquier revisión no autorizada, uso, copia, divulgación o distribución. Si usted no es el destinatario, por favor póngase en contacto con el remitente por correo electrónico de respuesta y destruir todas las copias del mensaje original. Gracias.

Confidentiality Notice: This e-mail message, including any attachments, sent on behalf of Liferay SL is for the sole use of the intended recipient(s) and may contain confidential and privileged information. Any unauthorized review, use, copying, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message. Thank you.

[drakonux]

Yeeee Finally! Is this going to be a variant of alert? 💙

Going with an alert is good for us too. The requirements, for now, are just a non-boxed alert.

Said that I can't predict if the same features of an alert would be needed. For now, reviewing the cases. I saw that the component is persistent (no closeable) in all the cases however it seems probable a product would need a closable non-boxed alert.

[matuzalemsteles]

Thanks @drakonux, well I have a tendency for this to be an alert variant the same as we did for 'stripe' in the design perspective do you think it makes sense?

I think that if these use cases arise, just adding as a variant of Alert will be easier because we will do the minimum implementation here to support this and we don't have to create a new component to cover this case because it is very similar to Alert and at the same time provide the same features that Alert offers. If behaviors arise outside the basic implementation of an alert, we can start to make a component available outside. Does it make sense?

[jbalsas]

I think you can make the call by now, @matuzalemsteles!! 💪

[matuzalemsteles]

@pat270 can you take care of implementing the additional CSS if necessary?

[pat270]

@matuzalemsteles Sure, I'm doing that now.

[pat270]

@drakonux @dgarciasarai I just send a pr for this. It won't cover the use case: I think for this one it's best to use utility classes. Something like:

<strong class="text-secondary">
  <span class="inline-item">Total Views</span>
  <span class="inline-item inline-item-after"><svg></svg></span>
  <span class="inline-item inline-item-after text-warning" title="The tooltip text"><svg></svg></span>
</strong>

[matuzalemsteles]

Well, I think we have reached a consensus here and we have both implementations merged into the master, the implementation on the CSS side, and the Alert component together with documentation for both.

• Component, Storybook, Docs: #4016
• CSS, Markup Docs: #4014

This will come out in the release of the 21st, see the milestone https://github.com/liferay/clay/milestone/40. Closing this now, thanks to everyone for the discussion!

[github-actions[bot]]

This issue has been merged and will be released in DXP at https://issues.liferay.com/browse/LPS-129362