conversionxl / aybolit

Lightweight web components library built with LitElement.
https://conversionxl.github.io/aybolit/
MIT License
7 stars 8 forks source link

feat(cxl-lumo-styles): style all links #417

Closed anoblet closed 3 months ago

anoblet commented 4 months ago

https://app.clickup.com/t/86b0kry6y

github-actions[bot] commented 4 months ago

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 76.15 KB (+0.05% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.89 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 28.99 KB (0%)
packages/cxl-ui/pkg/dist-web/vendor.js 140.5 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js 258.68 KB (+0.02% 🔺)
anoblet commented 4 months ago

I did a grep for a :visited beforehand. Unless styles are outside aybolit, this was my best guess.

anoblet commented 4 months ago

Does this not destroy state colors, like :visited?

I cannot be told to fix :visited links as well as respect them. I'm going to leave this PR until there is a clear objective.

lkraav commented 4 months ago

Does this not destroy state colors, like :visited?

I cannot be told to fix :visited links as well as respect them. I'm going to leave this PR until there is a clear objective.

Certainly, but CU task title seems to talk about only MD hero dark background? This rule would override everything everywhere, or am I missing something?

Regardless, ultimate boss to slay here I think is to figure out how to use Vaadin contrast tokens to have a single rule do the right thing automatically both on light and dark backgrounds. @conversionxl/web feel free to correct me if I'm wrong?

anoblet commented 4 months ago

When digging into the issue, the closest shadow root is cxl-certificate-header. It's styles import cxl-lumo-styles/scss/typography. We have no styles for a:visited or a:any-link inside of Aybolit. Rather than put out a fire, I thought it may be helpful to define this.

You're right, color could be a concern depending on the background color. I thought CXL red would look good, on white or black backgrounds, and text underline wouldn't be needed.

Should I put the fire out, and only fix this issue, or try to solve :visited links all around?

These days I feel I'll be shot either way.

pawelkmpt commented 4 months ago

When digging into the issue, the closest shadow root is cxl-certificate-header. It's styles import cxl-lumo-styles/scss/typography. We have no styles for a:visited or a:any-link inside of Aybolit. Rather than put out a fire, I thought it may be helpful to define this.

Global style is lumo, right? Does it need changing or another override with the same styles but in cxl-lumo-styles/scss/typography?

Screenshot 2024-06-03 at 09 13 24

You're right, color could be a concern depending on the background color. I thought CXL red would look good, on white or black backgrounds, and text underline wouldn't be needed.

This component has no "dark" theme indicator, so general light/dark-aware styles would not work anyway.

Should I put the fire out, and only fix this issue, or try to solve :visited links all around?

These days I feel I'll be shot either way.

Well, it depends. How other components solve it? Are a styles defined for each of them separately? Is cxl-certificate-header any different from them and someone just forgot add this rule?

In general task is to fix an issue in this place. So put out a fire. But you can suggest better solution. If proposed solution is quick, doesn't destroy other components and no time-consuming refactoring is needed - let's go for it.

If "fixing a link color" gonna take us hours just fix this one specific item and that's it.

anoblet commented 4 months ago

Updated to only target cxl-certificate-header.