Fabric 7: TypeScript 3.5 Updates (WIP)#9209
Fabric 7: TypeScript 3.5 Updates (WIP)#9209msft-github-bot merged 18 commits intomicrosoft:fabric-7from JasonGore:jg/ts3-5-rc
Conversation
| // It'd be great to properly type this while allowing 'aria-` and 'data-' attributes like TypeScript does for JSX attributes, | ||
| // but that ability is hardcoded into the TS compiler with no analog in TypeScript typings. | ||
| // Then we'd be able to enforce props extends native props (including aria- and data- attributes), and then return native props. | ||
| // We should be able to do this once this PR is merged: https://github.com/microsoft/TypeScript/pull/26797 |
JasonGore
requested review from
Jahnp,
KevinTCoughlin,
Markionium,
Vitalius1,
aneeshack4,
atneik,
dzearing,
joschect,
kenotron,
khmakoto,
kkjeer,
micahgodbolt,
mtennoe,
srideshpande and
yiminwu
as code owners
| */ | ||
| as: 'th' | 'td'; | ||
| // TODO: doc says default is 'td', where is it set? if it has a default, why is 'as' required? making optional for now. | ||
| as?: 'th' | 'td'; |
There was a problem hiding this comment.
@ecraig12345 FYI. The watch issue combined with my lack of knowledge with this code made it hard to iterate and find a good fix for this.
| export type IPageSectionPropsWithSectionName = IPageSectionProps; | ||
| // export type IPageSectionPropsWithSectionName<TPlatform extends string = string> = | ||
| // Required<Pick<IPageSectionProps<TPlatform>, 'sectionName'>> & Omit<IPageSectionProps<TPlatform>, 'sectionName'>; | ||
| // export type IPageSectionPropsWithSectionName = Required<Pick<IPageSectionProps, 'sectionName'>> & Omit<IPageSectionProps, 'sectionName'>; |
There was a problem hiding this comment.
@ecraig12345 FYI. The watch issue combined with my lack of knowledge with this code made it hard to iterate and find a good fix for this.
| examples: IExample[]; | ||
| // TODO: There seems to be a disparity between this type and IPageSectionProps as used in Page.tsx. | ||
| // Making optional for now to workaround. | ||
| examples?: IExample[]; |
|
CI (aside from CLA) appears not to be running against this PR. |
|
I'm not sure why CI on this PR remains busted. Perhaps it's because it started as a draft PR? I'm going to close this PR and create another. |
|
Component perf results:
|
|
|
||
| // TODO: Have Markus verify... className props was required, but I'm not sure if it should be. | ||
| // If it is truly required, it seems the parent component should be ensuring that default values are | ||
| // provided and not assume the consumer of the slot will provide them. |
There was a problem hiding this comment.
@Markionium FYI, wasn't sure which way to go here. (Sorry for the typo on your name.)
|
Hello @JasonGore! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
@msft-github-bot Please merge in 1 minute |
|
Hello @JasonGore! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
7 participants
Pull request checklist
$ npm run changeDescription of changes
WIP TS3.5 updates on
fabric-7branch.Microsoft Reviewers: Open in CodeFlow