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

Fix SnapGrid is almost invisible in light theme #85585

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

jsjtxietian
Copy link
Contributor

@jsjtxietian jsjtxietian commented Dec 1, 2023

Fixes #85574

cc @MewPurPur Any suggestions?

@jsjtxietian jsjtxietian requested a review from a team as a code owner December 1, 2023 04:18
@YeldhamDev YeldhamDev added this to the 4.x milestone Dec 1, 2023
@MewPurPur
Copy link
Contributor

MewPurPur commented Dec 1, 2023

This was my mistake, I mixed this up with work on a snap icon for my own app (which uses #def as its icon color). I don't think I even intended to change this.

Godot's icon color is e0e0e0, so please use that.

@jsjtxietian
Copy link
Contributor Author

Godot's icon color is e0e0e0, so please use that.

Thanks! I first use that too, but then find the other snap icon (Snap.svg) is #fff so I used that, should I change that too ?

@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Dec 1, 2023
@YuriSizov YuriSizov added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 1, 2023
@MewPurPur
Copy link
Contributor

MewPurPur commented Dec 1, 2023

I just checked the SVGs, it seems to me like those icons are likely also slightly broken in light theme. There's no reason not to use e0e0e0 for them. So yes, I think they should be changed.

@akien-mga
Copy link
Member

There seems to be a lot of icons using fill="#fff" (and some use #fff for stroke too.
If this should be changed to the remappable #e0e0e0, this might best be done in a dedicated PR. But I assume some of them use #fff intentionally to keep it white even on light theme (e.g. eyes of Godot logo).

$ rg -l 'fill="#fff"' | wc -l
87

@MewPurPur
Copy link
Contributor

Let me look into it.

@MewPurPur
Copy link
Contributor

MewPurPur commented Dec 5, 2023

I don't find anything else that needs to be changed. I should mention that #fefefe is the "forced white" color, white #fff becomes a dark gray when it's in light theme. And #e0e0e0 is the icon color, it's a very light gray, and in light theme it's a dark gray (but less dark than the one #fff becomes.)

A lot of the occurrences of #fff are outside editor/icons, which is correct and shouldn't be changed. In editor/icons, I also didn't find any occurrences of it being used for icons. It's commonly used for GUI elements (I consider it not icons, also opacity is used to determine how much those stick out) and for contrast, mainly for icons like Transform2D, which is just "T2D" as an icon and it uses a transparent white to add extra contrast to the middle letter, so as to unstick them and make the icon more readable. Also for icons like the overbright indicator, trying to have as much contrast as possible between white and black. These shouldn't be changed either (Actually, they could use #fefefe and get removed from the list of exceptions, but honestly it doesn't matter and it'll need extra care to make sure the other colors don't get messed up from it). Then there are the Godot logos using it for their eyes, obviously shouldn't be changed.

In short, I didn't find any other icons that need their fill color tweaked from #fff to #e0e0e0.

@akien-mga akien-mga merged commit 1af8228 into godotengine:master Dec 5, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.2.1.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 7, 2023
@jsjtxietian jsjtxietian deleted the fix-svg-invisible branch December 9, 2023 16:43
@YuriSizov YuriSizov changed the title Fix SnapGrid is almost invisble in light theme Fix SnapGrid is almost invisible in light theme Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Use Grid Snap" icon is hardly visible with Light theme
5 participants