Skip to content

Conversation

@wise-king-sullyman
Copy link
Collaborator

Closes #4059

@patternfly-build
Copy link
Collaborator

patternfly-build commented Jun 25, 2024

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM! Just one thing about the circle potentially disappearing if it's the same color as the background behind the circle. And another comment for later I imagine.

style={{ backgroundColor: value }}
/>
<Icon>
<CircleIcon color={value}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the previous circle had a border and box shadow, which we probably want to retain at least one of those for when the circle's color is the same or really close to the background color.

And this is probably something for later, but we're showing the hex value for the color both as the text in the table (#0066cc) but also as the color of the icon (color="#0066cc"), which isn't accurate in dark theme. Technically this applies to other global vars, too, but mostly colors. For colors, you could just use the component var the table row is referencing for the circle color since component vars are available at the :root document scope.

Screenshot 2024-06-27 at 8 57 51 PM Screenshot 2024-06-27 at 8 58 10 PM

@nicolethoen
Copy link
Collaborator

Seeing this, I think it may not be super accessible the way you've removed the hex values. They might need to be there?

@nicolethoen nicolethoen merged commit d5d07fc into patternfly:v6 Jul 11, 2024
@wise-king-sullyman wise-king-sullyman deleted the css-vars-styles branch July 12, 2024 13:12
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.

Review cssVariables.css styles for replacing custom styles with existing styling options or tokenization

4 participants