-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
IconNames: Deprecate due to const enum usage #8005
IconNames: Deprecate due to const enum usage #8005
Conversation
Hello @JasonGore! Because this pull request has the Do note that I've been instructed to only help merge pull-requests of this repository that have been opened for at least 8 hours, a condition that is not currently met. No worries though, I will be back when the time is right! 😉 |
…nGore/office-ui-fabric-react into jg/7110-deprecate-iconNames
Size Auditor did not detect a change in bundle size for any component! |
packages/icons/src/IconNames.ts
Outdated
@@ -1675,4 +1678,8 @@ export const enum IconNames { | |||
SizeLegacy = 'SizeLegacy' | |||
} | |||
|
|||
/** | |||
* @deprecate IconNames is deprecated. |
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.
Why is this deprecated? I don't think the type IconNamesInput
would need to be deprecated.
Also, everything in the @uifabric/icons
package is generated by the icon subsetting tool. You can talk with @Jahnp here, but we should change the tool first if we want to make any changes.
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 is dependent on an deprecated object, and could also result in the issue people are seeing in #7110, so that is why I also marked this deprecated. Does that make sense?
I will talk to Peter about the tool, thanks.
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.
More simply put maybe I should have just said it's a deprecated type, since it's keying off a deprecated object. 😄
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 believe @ThomasMichon added this. A better approach would be to keep the type, but just generate the type. adding this was a cheap way to get some type safety.
also, the tag should be @deprecated
, not @deprecate
.
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 removed this change before the PR was merged.
Last I checked, SPFx was using string union type, but will double check. cc: @patmill |
Spoke with @Jahnp and we may decide to close this PR and make another one from the upstream tool changes. I'll mark "Do not merge" for now and close/update within the next day or so. |
@JasonGore and I chatted some more offline. This works great for now--we'll go ahead and deprecate the |
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.
Spoke with SPFx offline, signing-off
🎉 Handy links: |
🎉 Handy links: |
@Jahnp, is there a road-map w/ ETA for the 7.0 release? |
@BTC-Bradley project planning for 7.0 is early but actively being worked on. We're hoping to have it out within the next few months, but that's pending some investigations. You can track progress here (project is being built up & managed by @jdhuntington): https://github.com/OfficeDev/office-ui-fabric-react/projects/29 |
Pull request checklist
$ npm run change
Description of changes
Per #7110 deprecate
IconNames
due to its usage ofconst enum
.Microsoft Reviewers: Open in CodeFlow