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

Update for specific site #12667

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update for specific site #12667

wants to merge 2 commits into from

Conversation

L11K
Copy link

@L11K L11K commented May 2, 2024

SVG icons were turning dark instead of remaining white, for better visibility, for both light and dark themes.

As seen here:
image image

SVG icons were turning dark instead of remaining white, for better visibility

As seen here: https://i.imgur.com/XXpYDtm.png

Expected result: https://i.imgur.com/NZsYGe9.png
@L11K L11K changed the title Update dynamic-theme-fixes.config Update for specific site May 2, 2024
@alexanderby
Copy link
Member

Hi @L11K, did it look fine before? Or did you notice the problem recently?

@alexanderby
Copy link
Member

If you look at these SVG icons in Chrome (or FireFox) Dev Tools, do you see role="img" attribute?

@L11K
Copy link
Author

L11K commented May 3, 2024

Hi @L11K, did it look fine before? Or did you notice the problem recently?

Hi,@alexanderby, It always looked like this, but I was too lazy to "fix" it before yesterday. Now I want my coworkers to use the extension too, which having to apply the "fix" to them one-by-one, as they aren't as techno-able.
image

@L11K
Copy link
Author

L11K commented May 3, 2024

If you look at these SVG icons in Chrome (or FireFox) Dev Tools, do you see role="img" attribute?

Not that I noticed, no
image

@alexanderby
Copy link
Member

I meant the <svg role="img">. I see it's there. Thanks for reporting, I will revert my change that causes the issue.

@L11K
Copy link
Author

L11K commented May 3, 2024

I meant the <svg role="img">. I see it's there. Thanks for reporting, I will revert my change that causes the issue.

Ah, yes, after data-icon. Thankfully even tho I didn't see, I posted the screenshot.

Thanks for your time and patience.

Copy link
Collaborator

@Myshor Myshor left a comment

Choose a reason for hiding this comment

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

Not starting tests until commit will be fixed.

src/config/dynamic-theme-fixes.config Show resolved Hide resolved
src/config/dynamic-theme-fixes.config Show resolved Hide resolved
Adjusting for consistency
@L11K L11K requested a review from Myshor May 4, 2024 15:17
@Myshor
Copy link
Collaborator

Myshor commented May 4, 2024

@alexanderby after reading your comments I think I will leave it to you if it should be merged or not.

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.

None yet

3 participants