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.

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

GroupHeader: tighten typings #8484

Merged
merged 4 commits into from
Mar 27, 2019

Conversation

natalieethell
Copy link
Contributor

@natalieethell natalieethell commented Mar 26, 2019

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

Tightens typings in GroupHeader, which is used in GroupedList. Right now, I'm pretty sure the prop isGroupLoading will never work. headerProps doesn't exist on IGroupHeaderProps, so the isGroupLoading const would have always been undefined.

The original IGroupHeaderProps had headerProps, but when that API was changed, this part of the code probably went unnoticed since it didn't have proper typing.

Focus areas to test

Ensure that IGroupHeaderProps's isGroupLoading (extended from IGroupDividerProps) works correctly now.

I'll see if I can write a test for this change, too.

Microsoft Reviewers: Open in CodeFlow

@size-auditor
Copy link

size-auditor bot commented Mar 27, 2019

Bundle test Size (minified) Diff from master
GroupedList 118.86 kB BelowBaseline     -19 bytes
ShimmeredDetailsList 219.625 kB BelowBaseline     -19 bytes
DetailsList 208.801 kB BelowBaseline     -19 bytes

ExceedsTolerance  Exceeds Tolerance     ExceedsBaseline  Exceeds Baseline     BelowBaseline  Below Baseline     1 kB = 1000 bytes

@dzearing dzearing merged commit 856dbd5 into microsoft:master Mar 27, 2019
@msft-github-bot
Copy link
Contributor

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

Handy links:

@natalieethell natalieethell deleted the tightenGroupedListTypings branch August 9, 2019 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants