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

feat(DetailsList): Support custom group header checkbox #13178

Closed
wants to merge 4 commits into from
Closed

feat(DetailsList): Support custom group header checkbox #13178

wants to merge 4 commits into from

Conversation

breezewish
Copy link

Pull request checklist

Description of changes

This PR adds onRenderGroupHeaderCheckbox to DetailsList to allow customizing the group header checkbox.

Focus areas to test

(optional)

@breezewish breezewish requested a review from dzearing as a code owner May 15, 2020 10:25
@msftclas
Copy link

msftclas commented May 15, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@breezewish breezewish mentioned this pull request May 15, 2020
Closed
@msft-github-bot
Copy link
Contributor

msft-github-bot commented May 15, 2020 •
edited
Loading

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Iterations Status
BaseButton 722 724 5000
Checkbox 1451 1443 5000
CheckboxBase 1171 1206 5000
CheckboxNext 1446 1410 5000
ChoiceGroup 4478 4468 5000
ComboBox 847 867 1000
CommandBar 6988 6941 1000
ContextualMenu 12816 12740 1000
DefaultButton 961 971 5000
DetailsRow 3092 3104 5000
DetailsRow (fast icons) 3075 3178 5000
DetailsRow without styles 2954 2936 5000
Dialog 1338 1323 1000
DocumentCardTitle with truncation 1528 1552 1000
Dropdown 2216 2191 5000
FocusZone 1574 1506 5000
IconButton 1529 1533 5000
Label 270 277 5000
Link 410 401 5000
LinkNext 387 402 5000
MenuButton 1309 1307 5000
Nav 2863 2880 1000
Panel 1279 1327 1000
Persona 754 789 1000
Pivot 1196 1231 1000
PrimaryButton 1102 1102 5000
SearchBox 1156 1154 5000
Slider 1408 1359 5000
Spinner 364 367 5000
SplitButton 2720 2718 5000
Stack 422 448 5000
Stack with Intrinsic children 1005 1059 5000
Stack with Text children 3850 3970 5000
TagPicker 2587 2423 5000
Text 338 339 5000
TextField 1203 1265 5000
Toggle 800 753 5000
button 75 61 5000

Perf Analysis (Fluent)

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
AccordionMinimalPerf.default 130 132 0.98:1 analysis
Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.45 0.44 1.02:1 2000 903
🦄 Button.Fluent 0.1 0.17 0.59:1 5000 500
🔧 Checkbox.Fluent 0.6 0.33 1.82:1 1000 600
🔧 Dialog.Fluent 0.33 0.2 1.65:1 5000 1643
🔧 Dropdown.Fluent 3.22 0.42 7.67:1 1000 3215
🔧 Icon.Fluent 0.13 0.04 3.25:1 5000 635
🎯 Image.Fluent 0.07 0.1 0.7:1 5000 343
🔧 Slider.Fluent 1.37 0.34 4.03:1 1000 1374
🔧 Text.Fluent 0.06 0.02 3:1 5000 305
🦄 Tooltip.Fluent 0.09 17.19 0.01:1 5000 431

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
DividerMinimalPerf.default 380 316 1.2:1
RefMinimalPerf.default 201 184 1.09:1
TreeWith60ListItems.default 221 205 1.08:1
ButtonSlotsPerf.default 581 544 1.07:1
DropdownMinimalPerf.default 3310 3101 1.07:1
PortalMinimalPerf.default 294 275 1.07:1
TreeMinimalPerf.default 1163 1093 1.06:1
AlertMinimalPerf.default 275 262 1.05:1
EmbedMinimalPerf.default 1994 1904 1.05:1
Image.Fluent 343 326 1.05:1
ItemLayoutMinimalPerf.default 1530 1473 1.04:1
MenuButtonMinimalPerf.default 1468 1418 1.04:1
ProviderMinimalPerf.default 639 614 1.04:1
CardMinimalPerf.default 501 485 1.03:1
LoaderMinimalPerf.default 715 694 1.03:1
TextAreaMinimalPerf.default 419 408 1.03:1
Button.Fluent 500 485 1.03:1
AvatarMinimalPerf.default 469 460 1.02:1
BoxMinimalPerf.default 294 287 1.02:1
ChatMinimalPerf.default 562 550 1.02:1
LayoutMinimalPerf.default 499 488 1.02:1
ListCommonPerf.default 902 883 1.02:1
ProviderMergeThemesPerf.default 1607 1582 1.02:1
ReactionMinimalPerf.default 351 343 1.02:1
DropdownManyItemsPerf.default 1258 1248 1.01:1
LabelMinimalPerf.default 360 356 1.01:1
RadioGroupMinimalPerf.default 518 511 1.01:1
Dropdown.Fluent 3215 3169 1.01:1
Slider.Fluent 1374 1357 1.01:1
Tooltip.Fluent 431 428 1.01:1
AnimationMinimalPerf.default 566 568 1:1
ChatWithPopoverPerf.default 491 493 1:1
FlexMinimalPerf.default 263 262 1:1
ListNestedPerf.default 823 823 1:1
SplitButtonMinimalPerf.default 3135 3140 1:1
IconMinimalPerf.default 628 628 1:1
CustomToolbarPrototype.default 3556 3561 1:1
ToolbarMinimalPerf.default 784 787 1:1
VideoMinimalPerf.default 565 563 1:1
CheckboxMinimalPerf.default 2710 2744 0.99:1
DialogMinimalPerf.default 1634 1643 0.99:1
HeaderSlotsPerf.default 1343 1361 0.99:1
InputMinimalPerf.default 934 939 0.99:1
ListMinimalPerf.default 427 433 0.99:1
SegmentMinimalPerf.default 306 308 0.99:1
TableMinimalPerf.default 332 335 0.99:1
Icon.Fluent 635 643 0.99:1
HierarchicalTreeMinimalPerf.default 891 906 0.98:1
ListWith60ListItems.default 1035 1059 0.98:1
MenuMinimalPerf.default 593 607 0.98:1
TextMinimalPerf.default 299 305 0.98:1
Avatar.Fluent 903 919 0.98:1
AttachmentSlotsPerf.default 1061 1095 0.97:1
ImageMinimalPerf.default 309 317 0.97:1
StatusMinimalPerf.default 606 624 0.97:1
Dialog.Fluent 1643 1701 0.97:1
ButtonMinimalPerf.default 153 160 0.96:1
ChatDuplicateMessagesPerf.default 365 382 0.96:1
HeaderMinimalPerf.default 429 448 0.96:1
SliderMinimalPerf.default 1301 1349 0.96:1
TooltipMinimalPerf.default 662 688 0.96:1
Checkbox.Fluent 600 627 0.96:1
Text.Fluent 305 318 0.96:1
CarouselMinimalPerf.default 422 446 0.95:1
GridMinimalPerf.default 605 634 0.95:1
PopupMinimalPerf.default 233 244 0.95:1
AttachmentMinimalPerf.default 134 143 0.94:1
FormMinimalPerf.default 337 364 0.93:1

@size-auditor
Copy link

size-auditor bot commented May 15, 2020 •
edited
Loading

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react office-ui-fabric-react-DetailsList 212.033 kB 212.361 kB ExceedsBaseline     328 bytes
office-ui-fabric-react fluentui-react-next-DetailsList 212.033 kB 212.361 kB ExceedsBaseline     328 bytes
office-ui-fabric-react office-ui-fabric-react-ShimmeredDetailsList 222.744 kB 223.072 kB ExceedsBaseline     328 bytes
office-ui-fabric-react fluentui-react-next-ShimmeredDetailsList 222.744 kB 223.072 kB ExceedsBaseline     328 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 96d987730064b7dbba4c785a3a1fa8c8f169958b (build)

@msft-github-bot
Copy link
Contributor

@ecraig12345

Gentle ping that this issue needs attention.

@JustSlone
Copy link
Collaborator

Assigning to code owner @dzearing to review

@JustSlone JustSlone assigned dzearing and unassigned ecraig12345 Jul 10, 2020
@JustSlone
Copy link
Collaborator

@ThomasMichon do you have any concerns taking this PR in DetailsList? it looks reasonable.

@breeswish the PR will need to be updated per the conflicts with master

ecraig12345
ecraig12345 approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

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

Not Thomas :) but I originally suggested this change in response to an issue, and it looks okay to me. Really all it's doing is exposing a previously-added GroupedList prop through DetailsList.

khmakoto
khmakoto previously requested changes Sep 28, 2020
View reviewed changes
Copy link
Member

@khmakoto khmakoto left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this fix, but due to work we're currently doing to prepare master for our version 8 beta release, we're asking contributors to either wait a couple weeks to submit fixes (if it's not urgent) or submit to the new 7.0 branch (if it's urgent). See #15222 for more details.

@msft-github-bot
Copy link
Contributor

This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI!

@breezewish
Copy link
Author

@khmakoto Thanks for the information! Since I'm also quite busy recently I will postpone conflicts resolution to weeks later after you have migrated the code base.

@layershifter
Copy link
Member

@khmakoto what is a resolution there? Should be a new PR opened or we can target 7.0 branch?

@khmakoto
Copy link
Member

khmakoto commented Jan 7, 2021

Thanks for the ping @layershifter, I think this can now go into master without problems, I'll dismiss my request for changes.

@khmakoto khmakoto dismissed their stale review January 7, 2021 22:27

No longer relevant.

@layershifter
Copy link
Member

@breeswish can you please solve merge conflicts? 🐱

@msft-fluent-ui-bot
Copy link
Collaborator

This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI!

@Onokaev
Copy link

Onokaev commented Aug 30, 2021

Hi @ecraig12345. Are there plans to merge this PR?

@maxosp
Copy link

maxosp commented Mar 18, 2022

Hi @ecraig12345, I think we are still waiting for merging this PR :) Or onRenderGroupHeaderCheckbox was already added to DetailsList props?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecraig12345 ecraig12345 ecraig12345 approved these changes

@khmakoto khmakoto khmakoto left review comments

@dzearing dzearing Awaiting requested review from dzearing

@ThomasMichon ThomasMichon Awaiting requested review from ThomasMichon

Assignees

@dzearing dzearing

Labels
Component: DetailsList Issue Pinged Needs: Author Feedback PR: API Modified
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DetailsList onRenderCheckbox is not effective for group headers