feat(@clayui/dropdown): LPD-50132 Adapt tests

[veroglez]

Hi guys!

According to this CMS story LPD-48264, we need the left icons of the dropdown items to support different display types, as shown in the following image:

After several discussions with the design teams (cc @marcoscv-work), it was decided that this should be contributed to Clay for standardization.

For that reason, I’m sending this pull request where I’ve added a new prop for the display type of the left icon, symbolLeftDisplayType.

Let me know what you think! 😊 Thanks in advance!

[marcoscv-work]

The visual design will be contributed at CX Theme level, here an example:
https://codepen.io/marcoscv/full/ogNxqKg

What looks like a sticker is not actually a sticker, it will be contributed to the CX theme simply by using a CSS modifier on the icon and in combination with this display type contributed by @veroglez

[ethib137]

Hey @veroglez and @marcoscv-work . Thanks for working on this.

I noticed that when the display type is danger the text changes in addition to the icon. This means that adding a class to the icon wouldn't be sufficient to restyle this fully for that case. We would need a class added to the dropdownItem div. For this, we already have the classname prop which would allow us to pass a class such as dropdown-item-{displayType}. With this class we could then add the styling changes necessary in the themeCSS CX.

I'm leaning towards, this being a better approach instead of adding another prop to the ClayDropdownItem. What do you all think? Is there a reason I'm not considering that would still make it better to add this to the ClayDropdownItem?

Thanks.

[marcoscv-work]

Hey @veroglez and @marcoscv-work . Thanks for working on this.

I noticed that when the display type is danger the text changes in addition to the icon. This means that adding a class to the icon wouldn't be sufficient to restyle this fully for that case. We would need a class added to the dropdownItem div. For this, we already have the classname prop which would allow us to pass a class such as dropdown-item-{displayType}. With this class we could then add the styling changes necessary in the themeCSS CX.

I'm leaning towards, this being a better approach instead of adding another prop to the ClayDropdownItem. What do you all think? Is there a reason I'm not considering that would still make it better to add this to the ClayDropdownItem?

Thanks.

Yeah, I think we were adding this prop for the cases we only need to customize the icon color:
https://www.figma.com/design/wACnRY2SPOFNrTBMTQ4Xb5/LPD-28835---Core-Capabilities-of-the-new-Structures-editor?node-id=1187-8141

For other functionalities we considered changing the icon color, it was not done because it was not technically possible, but of course, if there is a way to make the customization in the simplest way, let's do it.

[veroglez]

Hey @veroglez and @marcoscv-work . Thanks for working on this.
I noticed that when the display type is danger the text changes in addition to the icon. This means that adding a class to the icon wouldn't be sufficient to restyle this fully for that case. We would need a class added to the dropdownItem div. For this, we already have the classname prop which would allow us to pass a class such as dropdown-item-{displayType}. With this class we could then add the styling changes necessary in the themeCSS CX.
I'm leaning towards, this being a better approach instead of adding another prop to the ClayDropdownItem. What do you all think? Is there a reason I'm not considering that would still make it better to add this to the ClayDropdownItem?
Thanks.

Yeah, I think we were adding this prop for the cases we only need to customize the icon color: https://www.figma.com/design/wACnRY2SPOFNrTBMTQ4Xb5/LPD-28835---Core-Capabilities-of-the-new-Structures-editor?node-id=1187-8141

For other functionalities we considered changing the icon color, it was not done because it was not technically possible, but of course, if there is a way to make the customization in the simplest way, let's do it.

Hey @ethib137, as @marcoscv-work mentioned, our goal here is to allow customization of the icon, which is why we consider this the simplest way to implement it—by allowing a displayType to be passed to the left icon.

If the entire item needs customization, the className prop on ClayDropdownItem already provides this flexibility. For example, by passing text-{displayType}, which are classes that already exist in Clay. In other words, if the intention is to style the entire item, the existing className prop with text-{displayType} should be enough, while the new prop would target only the icon. We follow this approach because there are no predefined display types that apply exclusively to either the icon or the entire item.

Does this approach make sense to you? Do you think it would work well, or do you see any potential issues with it?

[ethib137]

Hey @veroglez , thanks for explaining that.

So I think if, I read between the lines, the reason why you want to add this prop is because you're trying not to add additional css. Is that right? Normally, I would be very much in favor for that approach, but for the new CMS, I don't think that's necessarily how we want to go about it.

We have already discussed that the new CMS will require a themeCSS CX that will handle the style changes required by the designers. Even this dropdown will need new CSS to be restyled to match the padding, border-radius, and text sizes of the designs. Our plan was that we would initially add most of those styles to the themeCSS CX and then evaluate if some of them made sense being brought back into the System.

Unless we have the plan of adding the ability to change the icon color of dropdown items in the Lexicon Library, I don't think we should be adding this to Clay.

Does that make sense or is there still something I'm missing?

[marcoscv-work]

Then we can put this PR on hold until Lexicon adds this feature, and until then, we will use the themeCSS CX for it. is it correct?
@veroglez @ethib137

[ethib137]

Yeah, that approach sounds good @marcoscv-work ! Let's do that. Thanks.

[veroglez]

yes! no problem. Go ahead with that approach @marcoscv-work. Thanks guys!

[ethib137]

Thanks everyone. I'm closing this for now.

[veroglez]

Reopened due to https://liferay.atlassian.net/browse/LPD-49622?focusedCommentId=2895876

[ethib137]

Let's still hold off on this until we add this to Lexicon not just as a satelite component. See here.