-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Calendar: support ID prop #25013
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
Calendar: support ID prop #25013
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f9716ee:
|
Asset size changes
Baseline commit: 0885025f43076ebdf3a395cfb6ae65b3304cc5b2 (build) |
📊 Bundle size report🤖 This report was generated against 0885025f43076ebdf3a395cfb6ae65b3304cc5b2 |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 1496 | 1567 | 5000 | |
| Breadcrumb | mount | 3536 | 3584 | 1000 | |
| Checkbox | mount | 3334 | 3386 | 5000 | |
| CheckboxBase | mount | 2973 | 2994 | 5000 | |
| ChoiceGroup | mount | 5695 | 5621 | 5000 | |
| ComboBox | mount | 1537 | 1557 | 1000 | |
| CommandBar | mount | 11741 | 11852 | 1000 | |
| ContextualMenu | mount | 12977 | 13123 | 1000 | |
| DefaultButton | mount | 1781 | 1766 | 5000 | |
| DetailsRow | mount | 4565 | 4504 | 5000 | |
| DetailsRowFast | mount | 4762 | 4540 | 5000 | |
| DetailsRowNoStyles | mount | 4387 | 4530 | 5000 | |
| Dialog | mount | 3816 | 3925 | 1000 | |
| DocumentCardTitle | mount | 704 | 726 | 1000 | |
| Dropdown | mount | 4188 | 4078 | 5000 | |
| FocusTrapZone | mount | 2550 | 2480 | 5000 | |
| FocusZone | mount | 2503 | 2353 | 5000 | |
| GroupedList | mount | 66641 | 78000 | 2 | |
| GroupedList | virtual-rerender | 32256 | 31224 | 2 | |
| GroupedList | virtual-rerender-with-unmount | 112845 | 111958 | 2 | |
| GroupedListV2 | mount | 709 | 715 | 2 | |
| GroupedListV2 | virtual-rerender | 667 | 683 | 2 | |
| GroupedListV2 | virtual-rerender-with-unmount | 704 | 718 | 2 | |
| IconButton | mount | 2549 | 2536 | 5000 | |
| Label | mount | 897 | 915 | 5000 | |
| Layer | mount | 5332 | 5438 | 5000 | |
| Link | mount | 1052 | 1080 | 5000 | |
| MenuButton | mount | 2193 | 2204 | 5000 | |
| MessageBar | mount | 3058 | 2917 | 5000 | |
| Nav | mount | 4170 | 4190 | 1000 | |
| OverflowSet | mount | 1691 | 1714 | 5000 | |
| Panel | mount | 3211 | 3148 | 1000 | |
| Persona | mount | 1580 | 1674 | 1000 | |
| Pivot | mount | 2092 | 2092 | 1000 | |
| PrimaryButton | mount | 1954 | 1960 | 5000 | |
| Rating | mount | 9425 | 9444 | 5000 | |
| SearchBox | mount | 1937 | 1989 | 5000 | |
| Shimmer | mount | 3738 | 3687 | 5000 | |
| Slider | mount | 2680 | 2664 | 5000 | |
| SpinButton | mount | 6484 | 5937 | 5000 | |
| Spinner | mount | 1004 | 984 | 5000 | |
| SplitButton | mount | 3968 | 4015 | 5000 | |
| Stack | mount | 1105 | 1093 | 5000 | |
| StackWithIntrinsicChildren | mount | 3148 | 3118 | 5000 | |
| StackWithTextChildren | mount | 6555 | 6448 | 5000 | |
| SwatchColorPicker | mount | 13474 | 13503 | 5000 | |
| TagPicker | mount | 3360 | 3412 | 5000 | |
| TeachingBubble | mount | 97086 | 98221 | 5000 | |
| Text | mount | 1027 | 1041 | 5000 | |
| TextField | mount | 2025 | 2039 | 5000 | |
| ThemeProvider | mount | 1943 | 1866 | 5000 | |
| ThemeProvider | virtual-rerender | 1316 | 1347 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 2700 | 2707 | 5000 | |
| Toggle | mount | 1399 | 1402 | 5000 | |
| buttonNative | mount | 703 | 672 | 5000 |
change/@fluentui-react-170d6594-4aee-4504-8885-b70ae2901269.json
Outdated
Show resolved
Hide resolved
Co-authored-by: Makoto Morimoto <[email protected]>
|
|
||
| return ( | ||
| <div | ||
| id={id} |
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 think v7 CalendarBase never actually applied id to the div. The forwarded ref has the HTMLDivElement props, but not the ICalendarProps. I think we could add id to v8, but then ODSP would see an output change when moving to v8.
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 checked it before, and the ID does get passed through. Here's a demo
* add ID to calendar types and pass to root div * change file * api update * Update change/@fluentui-react-170d6594-4aee-4504-8885-b70ae2901269.json Co-authored-by: Makoto Morimoto <[email protected]> Co-authored-by: Makoto Morimoto <[email protected]>
* add ID to calendar types and pass to root div * change file * api update * Update change/@fluentui-react-170d6594-4aee-4504-8885-b70ae2901269.json Co-authored-by: Makoto Morimoto <[email protected]> Co-authored-by: Makoto Morimoto <[email protected]>
v8 calendar does not support ID prop (and v7 does). This fix helps with migration as well as consistency between components.