Airplane icon added (angled) #497
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 4ecfbb0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
cypress-design
|
||||||||||||||||||||||||||||||||||||||||
| Project |
cypress-design
|
| Branch Review |
icon/share
|
| Run status |
|
| Run duration | 02m 35s |
| Commit |
|
| Committer | Emil Milanov |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
222
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
11.36%
|
|
|---|---|
|
Untested elements
|
166
|
|
Tested elements
|
25
|
Accessibility
99.37%
|
|
|---|---|
|
Failed rules
|
0 critical
1 serious
0 moderate
1 minor
|
|
Failed elements
|
22
|
.changeset/fluffy-falcons-refuse.md
Outdated
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@cypress-design/react-icon': minor | |||
There was a problem hiding this comment.
@emilmilanov I believe that the icon updates were considered as patch updates, instead of minor typically. We should probably continue to follow that pattern. Everything else looks good though.
There was a problem hiding this comment.
In looking at older PRs that introduce new icons, I saw both minor and patch used.
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.
I used yarn changeset to create this part of the code; it doesn't offer a patch option, 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 used minor.
There was a problem hiding this comment.
@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 patch updates 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.
@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 use patch as well for all three packages.
There was a problem hiding this comment.
Changeset is updated from minor to patch. I also renamed icons to object-airplane-angled...
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)
There was a problem hiding this comment.
@emilmilanov The code looks good. Do you mind just checking through the Percy screenshots as well? Most likely it's nothing and that build can be approved to before this merges in.
Let me know if it's largely flaky results. There's probably something we can do to reduce the flaky results in the future.
Share icon is added
