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

Icon: Remove hardcoded color from ai icon #87996

Merged
merged 5 commits into from
May 20, 2024
Merged

Icon: Remove hardcoded color from ai icon #87996

merged 5 commits into from
May 20, 2024

Conversation

ashharrison90
Copy link
Contributor

@ashharrison90 ashharrison90 commented May 16, 2024

What is this feature?

  • removes the hardcoded color from the ai icon and leaves it up to the consumer to implement
  • there are 2 places this will affect in the grafana org:
    • GenAIButton these are all used within primary buttons anyway, so they get the color reapplied for free ✅
    • ExplanationButton and Explanation in the ml app. this will change to white:
      before image
      after image
      • the workaround would be to pass the full icon component in to the button and specify the color, so:
      // before
      <Button icon="ai" ... />
      
      // after
      <Button icon={<Icon name="ai" color={theme.colors.primary.main} />} ... />
      • have raised it with the ML team and they're happy to either leave it white or make the change 👍

Why do we need this feature?

  • so people can set the color of the ai icon correctly

Who is this feature for?

  • everyone

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@ashharrison90 ashharrison90 added type/chore no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels May 16, 2024
@ashharrison90 ashharrison90 added this to the 11.1.x milestone May 16, 2024
@ashharrison90 ashharrison90 requested a review from a team May 16, 2024 16:15
@ashharrison90 ashharrison90 self-assigned this May 16, 2024
@ashharrison90 ashharrison90 requested a review from a team as a code owner May 16, 2024 16:15
@ashharrison90 ashharrison90 requested review from Clarity-89 and L-M-K-B and removed request for a team May 16, 2024 16:15
</linearGradient>
</defs>
<svg viewBox="0 0 48 48" fill="none" xmlns="http://www.w3.org/2000/svg">
<path d="M37.3333 24C27.0252 24 18.6675 34.7456 18.6675 48C18.6675 34.7456 10.3098 24 0 24C10.3098 24 18.6675 13.256 18.6675 0C18.6675 13.256 27.0252 24 37.3333 24Z" fill="currentColor" fill-opacity="0.65"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the shapes still be partly transparent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question - this is the icon i was given, so i guess not? 🤔

cc @staton-hyse11

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to unify with the rest of our icons and have it solid - no transparencies

@ashharrison90
Copy link
Contributor Author

@Clarity-89 do i also have to update this icon in the @grafana/icons package? 🤔

@Clarity-89
Copy link
Contributor

@Clarity-89 do i also have to update this icon in the @grafana/icons package? 🤔

You can replace the file in the package's svg folder with the updated svg and it'd be enough.

@ashharrison90 ashharrison90 requested review from a team as code owners May 20, 2024 08:53
@ashharrison90 ashharrison90 requested review from mckn and removed request for a team May 20, 2024 08:53
@joshhunt joshhunt enabled auto-merge (squash) May 20, 2024 08:56
@joshhunt joshhunt disabled auto-merge May 20, 2024 08:56
@ashharrison90 ashharrison90 merged commit 8f6dcfe into main May 20, 2024
17 checks passed
@ashharrison90 ashharrison90 deleted the ash/fix-ai-icon branch May 20, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes type/chore
Projects
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants