Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Conversation

anoblet
Copy link
Collaborator

@anoblet anoblet commented May 30, 2024

Copy link

github-actions bot commented May 30, 2024

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% 🔺)

Copy link

@lkraav lkraav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this not destroy state colors, like :visited?

@anoblet
Copy link
Collaborator Author

anoblet commented May 31, 2024

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

@anoblet
Copy link
Collaborator Author

anoblet commented May 31, 2024

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
Copy link

lkraav commented May 31, 2024

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
Copy link
Collaborator Author

anoblet commented May 31, 2024

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
Copy link

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
Copy link
Collaborator Author

anoblet commented Jun 4, 2024

Updated to only target cxl-certificate-header.

Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Color is fine, now we need underline on hover

@pawelkmpt pawelkmpt merged commit 17713d2 into master Jun 13, 2024
5 checks passed
@pawelkmpt pawelkmpt deleted the anoblet/feat/link branch June 13, 2024 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants