💄 Set proper foreground & hover colors for the device selector action #8576
Conversation
| g2.setColor(JBUI.CurrentTheme.ActionButton.hoverBackground()); | ||
| // Use MainToolbar hover background to adapt to the toolbar theme (e.g., dark header with light theme) | ||
| final JBColor hoverColor = JBColor.namedColor( | ||
| "MainToolbar.Icon.hoverBackground", |
There was a problem hiding this comment.
I'm nervous about having plain strings to identify colors in these spots - do you (or AI) know how we can find these key names in case they change in the future? What happens if the key is not findable? I wonder if we could instead pull out smaller functions like getHoverColor and add unit tests to make sure we're able to get a reasonable color?
Separately, if there are IJ platform constants that we could use here instead, that would be ideal. If not, then let's at least pull out as our own constants in this class.
There was a problem hiding this comment.
I'm nervous about having plain strings to identify colors in these spots - do you (or AI) know how we can find these key names in case they change in the future?
It seems to find keys from https://github.com/JetBrains/intellij-community/blob/idea/252.26830.84/platform/platform-resources/src/themes/HighContrast.theme.json. I didn't see a clear documentation that declares color properties. But I've added some descriptions to explain the behavior clearly.
What happens if the key is not findable?
It will fall back to a default color value.
I wonder if we could instead pull out smaller functions like
getHoverColorand add unit tests to make sure we're able to get a reasonable color?
I've added some unit tests.
Separately, if there are IJ platform constants that we could use here instead, that would be ideal. If not, then let's at least pull out as our own constants in this class.
Unfortunately, from my understanding and during my code, I found no accurate constant members for these colors.
| * Theme property key for the main toolbar foreground color. | ||
| * This key is used to retrieve the appropriate text color for toolbar components, | ||
| * ensuring proper visibility in all theme configurations (e.g., light theme with dark header). | ||
| * If this key is not found in the current theme, falls back to the standard label foreground color. |
There was a problem hiding this comment.
Let's take out this last line about the fallback as this is mentioned in the method that uses the key and isn't relevant to the key delcaration
| * Theme property key for the main toolbar icon hover background color. | ||
| * This key is used to retrieve the appropriate hover background color for toolbar icon buttons, | ||
| * ensuring consistency with other toolbar actions in all theme configurations. | ||
| * If this key is not found in the current theme, falls back to the standard action button hover background. |
There was a problem hiding this comment.
Take out this fallback comment as well
| } | ||
|
|
||
| /** | ||
| * Invokes the private static getToolbarHoverBackgroundColor method via reflection. |
There was a problem hiding this comment.
Is it necessary to make these static methods? Could this test file be simpler if we made them non-static methods?
2 participants
Fixes #8572
The request sets the foreground color and hover color consistently across all circumstances.
Contribution guidelines:
dart format.