Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move Calendar and DatePicker to legacy package #9253

Merged
merged 6 commits into from
May 30, 2019
Merged

Move Calendar and DatePicker to legacy package #9253

merged 6 commits into from
May 30, 2019

Conversation

ecraig12345
Copy link
Member

@ecraig12345 ecraig12345 commented May 29, 2019 •
edited
Loading

Pull request checklist

Description of changes

Move the old Calendar and DatePicker components to the legacy package. Also fixed API doc generation for the legacy package.

Microsoft Reviewers: Open in CodeFlow

@ecraig12345 ecraig12345 requested a review from natalieethell May 29, 2019 00:54
@ecraig12345 ecraig12345 requested a review from jdhuntington May 29, 2019 01:11
@msft-github-bot
Copy link
Contributor

msft-github-bot commented May 29, 2019 •
edited
Loading

Component perf results:

Scenario Target branch avg total (ms) PR avg total (ms) Target branch avg per item (ms) PR avg per item (ms) Is significant change Is regression
PrimaryButton 88.951 82.259 0.889 0.823 true false
DefaultButton ... 76.522 ... 0.765 ... ...
MenuButton ... 97.225 ... 0.972 ... ...
SplitButton ... 182.523 ... 1.825 ... ...
BaseButton ... 34.514 ... 0.345 ... ...
NewDefaultButton ... 84.338 ... 0.843 ... ...
NewPrimaryButton ... 83.967 ... 0.840 ... ...
NewMenuButton ... 148.064 ... 1.481 ... ...
NewSplitButton ... 287.428 ... 2.874 ... ...
button ... 6.159 ... 0.062 ... ...
DetailsRows without styles ... 181.451 ... 1.815 ... ...
DetailsRows ... 181.967 ... 1.820 ... ...
Toggles ... 41.416 ... 0.414 ... ...
NewToggle ... 66.281 ... 0.663 ... ...
DocumentCardTitle with truncation ... 23.841 ... 0.238 ... ...

@kenotron
Copy link
Member

I think you might need to update some things on the perf-test? Some of these things are blank.

@ecraig12345
Copy link
Member Author

Given that I didn't touch buttons, I doubt those are my fault. Maybe it's related to @khmakoto's recent PR touching Button in Fabric 7?

@khmakoto
Copy link
Member

Yes, I think those Button's are related to my PR. I'll check on why those entries are appearing as blanks.

@kenotron
Copy link
Member

Chatted offline with @jdhuntington about these deprecation. Was going to comment about lacking code mods but realized those are all separate work items. So that's fine! I was also hoping that there would be a dedicated "legacy" section in the website for these components so people know not to choose them if they can help it -also another work item :)

natalieethell
natalieethell requested changes May 29, 2019
View reviewed changes
Copy link
Contributor

@natalieethell natalieethell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually removing the Implementation sections for everything except Calender and DatePicker.

image

I have an idea of why this is - I'll look into it and get back to you.

@ecraig12345
Copy link
Member Author

@natalieethell Oops, I checked the Implementation sections for Calendar/DatePicker/Nav but assumed everything else would still work... Thanks for catching that!

@natalieethell
Copy link
Contributor

Looks like these lines are the culprit.

https://github.com/OfficeDev/office-ui-fabric-react/blob/0c51f3d0187279e5a86f011281035db51a7e56ef/packages/api-docs/src/PageJsonGenerator.tsx#L97-L99

Since this is in the following for loop

https://github.com/OfficeDev/office-ui-fabric-react/blob/0c51f3d0187279e5a86f011281035db51a7e56ef/packages/api-docs/src/PageJsonGenerator.tsx#L85

the page data associated with each page name is being overwritten with the addition of the legacy apiJsonPath. This isn't an issue for references, because there aren't any pageNames in that blob:

https://github.com/OfficeDev/office-ui-fabric-react/blob/0c51f3d0187279e5a86f011281035db51a7e56ef/packages/api-docs/src/generatePageJsonFiles.ts#L14

@natalieethell
Copy link
Contributor

Okay here's what I think will work:

We can move the following lines

https://github.com/OfficeDev/office-ui-fabric-react/blob/0c51f3d0187279e5a86f011281035db51a7e56ef/packages/api-docs/src/PageJsonGenerator.tsx#L96-L99

out of the for loop, so right above line 85

https://github.com/OfficeDev/office-ui-fabric-react/blob/0c51f3d0187279e5a86f011281035db51a7e56ef/packages/api-docs/src/PageJsonGenerator.tsx#L85

The pageNames object should be the same for each of the apiJsonPaths, so it doesn't need to happen inside the for loop.

@ecraig12345 ecraig12345 merged commit 5e4f160 into microsoft:fabric-7 May 30, 2019
@ecraig12345 ecraig12345 deleted the calendar branch May 30, 2019 05:20
@msft-github-bot
Copy link
Contributor

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-github-bot
Copy link
Contributor

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-github-bot
Copy link
Contributor

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-github-bot
Copy link
Contributor

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-github-bot msft-github-bot mentioned this pull request Jun 12, 2019
Closed
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@natalieethell natalieethell natalieethell approved these changes

@lorejoh12 lorejoh12 lorejoh12 approved these changes

@kenotron kenotron kenotron approved these changes

@dzearing dzearing Awaiting requested review from dzearing

@evlevy evlevy Awaiting requested review from evlevy

@Jahnp Jahnp Awaiting requested review from Jahnp

@jordandrako jordandrako Awaiting requested review from jordandrako

@jdhuntington jdhuntington Awaiting requested review from jdhuntington

Assignees

@kenotron kenotron

Labels
PR: API Modified
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ecraig12345 @msft-github-bot @kenotron @khmakoto @natalieethell @lorejoh12