-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat(icons): add holdout icon #1297
Conversation
🦋 Changeset detectedLatest commit: 0fd639b The changes in this PR will be included in the next version bump. This PR includes changesets to release 25 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Change: +2.77 kB (+1.39%) Total Size: 202 kB
ℹ️ View Unchanged
|
packages/icons/src/img/sprite.svg
Outdated
@@ -1983,5 +1983,10 @@ | |||
clip-rule="evenodd" | |||
/> | |||
</symbol> | |||
<symbol viewBox="0 0 20 20" id="lp-icon-holdout"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be awesome if you'd be willing to put this icon in alphabetical order like the rest of them! 🙏🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry about that, I will move into the correct order!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than moving the symbol in the sprite file to be in alphabetical order, though I don't have any context as to whether the icon itself is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits
- move holdout symbol to alphabetical
- each attribute should be on its own line if there are more than 1 (wish our prettier implementation just fixed this for you 😿 )
The svg path looks to be just a touch off-center. not a huge deal, but ideally you can swap the path definition for:
<symbol viewBox="0 0 20 20" id="lp-icon-holdout">
<path
fill-rule="evenodd"
d="M13.75 9.984a1.15 1.15 0 1 0 0 2.3 1.15 1.15 0 0 0 0-2.3Zm-2.65 1.15a2.65 2.65 0 1 1 5.3 0 2.65 2.65 0 0 1-5.3 0Z"
clip-rule="evenodd"
/>
<path d="M9.917 14.067a2.4 2.4 0 1 1-4.8 0 2.4 2.4 0 0 1 4.8 0Z"/>
<path
fill-rule="evenodd"
d="M6.75 4.984a1.65 1.65 0 1 0 0 3.3 1.65 1.65 0 0 0 0-3.3ZM3.6 6.634a3.15 3.15 0 1 1 6.3 0 3.15 3.15 0 0 1-6.3 0Z"
clip-rule="evenodd"
/>
</symbol>
@matthewferry @pheggeseth Thanks so much yall for reviewing, I moved the svg up to the correct order and swapped the path definition ✅ |
Summary
This PR adds the new icon 'holdout'. It will be used to represent a holdout object in the experimentation product.
Screenshots (if appropriate):
Testing approaches