-
Notifications
You must be signed in to change notification settings - Fork 1
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: add new icons #63
Conversation
We have a torget-shopping icon that looks exactly like the new shopping-cart icon. Removing the new icon until it's settled which icon should be valid.
default-icon-descriptions.js
Outdated
@@ -2044,11 +2084,6 @@ export default { | |||
id: "icon.title.shoes", | |||
comment: "Title for shoes icon" | |||
}, | |||
shoppingcart: { |
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.
Is this icon removed or something?
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.
Previously the description was there but the icon was not. When I added it from the new collection I saw that it looked exactly like "torget-shopping". So I removed all mentions of "shopping-cart" until I clarified what to do here.
I have just confirmed with Henrik that this new shopping-cart icon should in fact replace the "torget-shopping", but whether or not I can introduce such breaking change is not easy to check at the moment. I tried code search with different variations of "torget shopping" but don't know if I can trust the results 😄 Wdyt I should do?
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.
Here I added the new "ShoppingCart" icon: 87e5a63
I'm thinking that we could at least filter out TorgetShopping from the Icons table in the tech-docs. This way we could avoid introducing a breaking change by removing that icon completely from this package, while at the same time we'd limit the use of that icon.
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, except one comment
This reverts commit 9266200.
# [1.4.0-next.1](v1.3.0...v1.4.0-next.1) (2023-12-05) ### Features * add new icons ([#63](#63)) ([55fd810](55fd810))
# [1.4.0](v1.3.0...v1.4.0) (2023-12-12) ### Bug Fixes * remove messages-filled mask remains and other masks ([#66](#66)) ([e4d9112](e4d9112)) * update market and twitter icons ([#65](#65)) ([b9360bb](b9360bb)) ### Features * add new icons ([#63](#63)) ([55fd810](55fd810)) * add sweater and baby-onesie icons ([#68](#68)) ([c3d10b5](c3d10b5))
Adds new icons requested as part of WARP-405.
Further updates to icons which are part of the ticket will be handled in a separate PR.
42 (marketplace icons):
16/24/32: