Fabric 7: TypeScript 3.5 Updates (WIP) #9209
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
aneeshack4,
atneik,
dzearing,
Jahnp,
joschect,
kenotron,
KevinTCoughlin,
khmakoto,
kkjeer,
Markionium,
micahgodbolt,
mtennoe,
srideshpande,
Vitalius1 and
yiminwu
as code owners
| @@ -15,7 +15,8 @@ export interface IMarkdownTableCellProps extends IMarkdownTableProps { | |||
| * Render the table cell as a th or td. | |||
| * @default 'td' | |||
| */ | |||
| 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[]; |
There was a problem hiding this comment.
@ecraig12345 One more...
|
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". |
Pull request checklist
$ npm run changeDescription of changes
WIP TS3.5 updates on
fabric-7branch.Microsoft Reviewers: Open in CodeFlow