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

Adding generic and geo ML job icons #8248

Merged
merged 5 commits into from
Jan 7, 2025
Merged

Conversation

rbrtj
Copy link
Contributor

@rbrtj rbrtj commented Dec 18, 2024

During the Borealis review, we discovered that some icons in the anomaly detection job wizard do not properly respect dark mode:

image

Currently, these icons live in the Kibana repo. However, it would be beneficial to include them in EUI to prevent similar issues in the future.

image

General checklist:

  • Checked in both light and dark modes
  • Added or updated jest tests
  • A changelog entry exists and is marked appropriately.

@mgadewoll
Copy link
Contributor

@MichaelMarcialis @ryankeairns Are we ok to move those icons to EUI or are they consumer specific?

@peteharverson
Copy link

@MichaelMarcialis @ryankeairns are we good with @rbrtj adding these into EUI to go with the existing icons for the other types of ML anomaly detection jobs? The two new icons are used in the same places as the other job type icons, so it would be good to have these all available inside EUI.

@MichaelMarcialis
Copy link
Contributor

Yes, I think it would be fine to add these icons to EUI in the short term. Let me know if ya'll need any help in making a PR for that.

Longer term, we'll probably want to have a discussion about the future of these duo-tone icons and guidelines for their use and whether they are needed after the refresh of the basic glyph icons.

@rbrtj
Copy link
Contributor Author

rbrtj commented Jan 7, 2025

Hey @MichaelMarcialis, thanks for confirming!
I've just updated the changelog, and the PR is ready for review!

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @rbrtj

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 The changes look good to me. The new icons are correctly added and available in all our docs environments 👍 Thanks for contributing!

@@ -0,0 +1,9 @@
<svg width="32" height="32" viewBox="0 0 32 32" xmlns="http://www.w3.org/2000/svg">
Copy link
Contributor

Choose a reason for hiding this comment

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

General note, no change requested:
Generally icons should be 16px, afaik (docs) but I see that all "ml" icons are 32px by default, so this should be correct.

@mgadewoll mgadewoll merged commit 2b08104 into elastic:main Jan 7, 2025
5 checks passed
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.

6 participants