Airplane icon added (angled) #497
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@cypress-design/react-icon': minor | ||
| '@cypress-design/vue-icon': minor | ||
| '@cypress-design/icon-registry': minor | ||
| --- | ||
|
|
||
| Share icon added | ||
|
There was a problem hiding this comment. @emilmilanov Instead of just "share" as the name, would it make sense to call them "share-airplane" or something like that in case we ever add different icons for "share"? There was a problem hiding this comment. I think I agree on that. I do think that the icons should probably just reflect the name of whatever it is that has been illustrated instead of a semantic name. (i.e. airplane, airplane-angled, etc) |
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@emilmilanov I believe that the icon updates were considered as
patchupdates, instead ofminortypically. We should probably continue to follow that pattern. Everything else looks good though.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.
In looking at older PRs that introduce new icons, I saw both
minorandpatchused.From a semantic versioning definition, I think this type of change would align with
minor: "when you add functionality in a backward compatible manner"But I think you could argue either way.
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.
I used
yarn changesetto create this part of the code; it doesn't offer apatchoption, only major or minor. I can override this to patch if we agree on it. When I added icons to the DS before, I always usedminor.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.
@warrensplayer I'll let you make the final call on that. I'd rather just make sure we know which pattern to use and to stick with that. If you've got strong feelings on the matter, we'll just follow your lead.
That said, I kind of just guessed that bart was using
patchupdates more often as to not run out of minor numbers. I don't know how many numbers are available before a major version is required. Maybe it's endless?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.
@ryanjwilke There is no limit to the numbers.
@emilmilanov I did go back and look a little bit harder and the CHANGELOG.md files and it does seem like most of the changes that add icons use
patch. So I guess update your changeset file to usepatchas well for all three packages.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.
Changeset is updated from
minortopatch. I also renamed icons to object-airplane-angled...