Menu item text color is not set

[JonasDov]

After updating iTwinUI react from 1.23.2 to 1.37.1 menu item text color is not explicitly set. As we were wraping menu item with anchor, now we need to explicitly override anchor text color. Before: After:

[mayank99]

Hey @JonasDov, thanks for creating this issue.

Your DOM structure seems invalid. <a> should not be a parent of <li>, it should be a child. Also, our menu-item styles were recently changed to either inherit text colors or set them for active variants (see demo).

So this does not look like a bug in itwinui-css. If it "worked" in a prior version, it was an unintended side effect and not a feature. If you need to look different, I would recommend adding some overrides on your end.

[veekeys]

Huh? Im not gonna raise the issue, that we do not provide ability to use menu item as anchor (they have no other way to make whole menu item clickable as anchor), but even if anchor was inner child, our color inheritance idea is wrong. Now it expects to inherit it from menu, which would be broken if there is anchor in any inner elements.

[mayank99]

we do not provide ability to use menu item as anchor (they have no other way to make whole menu item clickable as anchor)

This is a separate issue (a valid one!) that should probably be opened in iTwinUI-react. I think we might want to expose a built-in way to use <li><a>...</a></li> with proper styles. We could add a prop to MenuItem or a new component like MenuItemLink.

our color inheritance idea is wrong

This was done for MenuExtraContent. See https://github.com/iTwin/iTwinUI/issues/368 for more context.

broken if there is anchor in any inner elements.

Anchor comes with its own styling so this is expected. I do not think this part has even changed between itwinui-react v1.23.2 and v1.37.1.

And I don't consider this "broken". To me it makes sense that the user needs to unset/override the color and text-decoration of <a> if those styles are undesirable.

[veekeys]

I know this change and I am saying, we might need to rethink the idea. From recent learning, I think all elements with text should have their color set explicitly. In this case, there is no situation we want our text to appear in different color in menu item unless user explicitly overrides it with css. Another example would be also connected with multiple themes: Imagine your whole page uses light theme, but you want your header to be dark. We change variables values to dark themes ones. Our typography elements do not have text color set explicitly (we expect those to be inherited). In this situation happens, that they inherit text color from body and body's text color is the light theme one even though in header context we do have other values assigned to the css variables. My question is, do we have the case, that having color on our elements with text would break something?

[FlyersPh9]

@mayank99 can this be addressed in #718? Or should this be addressed sooner than iTwinUI-CSS 1.0?

[mayank99]

i don't think this will be affected by the new variables or 1.0 at all, it's a problem confined to our component

[FlyersPh9]

I meant with our changing of hover (shades of gray) and active (shades of blue) in #718 does it make sense to include this fix with that change, but I don't think that this fix needs to be tied with that change in 718. I'll submit a fix to main now.

[FlyersPh9]

Fixed in #741.