-
Notifications
You must be signed in to change notification settings - Fork 387
Add Asset title text outline #1952
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
base: main
Are you sure you want to change the base?
Conversation
I think we should figure out why the 50% opacity black background for the title div is not working in some environments. |
I think that this PR should be considered on its own as a way to improve assets legibility (I personally don't think it's necessary, but maybe other people think otherwise) not as a solution for #1951. #1951 has a wider impact than the issue we've noticed in the assets and the correct solution is #1953. |
@@ -44,6 +44,7 @@ $asset-card-padding: 0.4rem; | |||
font-size: 1.2rem; | |||
background-color: var(--asset-card-title-bg-color); | |||
color: var(--asset-card-title-color); | |||
text-shadow: -1px 0 black, 0 1px black, 1px 0 black, 0 -1px black; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, colors shouldn't be used directly, define a --asset-cart-title-text-shadow-color
(verbose… 🤮) in _vars.scss
in its corresponding section. The reason is that this will allow to change the shadow color for light mode.
@rparrett FYI, in case you see a similar issue in other PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, no need to do the change unless we agree that we want the text shadow.
Closes #1951