Factor out an inner version of DetailsList which uses hooks #12870
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Asset size changes
Baseline commit: 234afcd191b0555f88c90c874dc341f8e1cf9666 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Potential regressions comparing to master
Perf comparison
Perf tests with no regressions
|
dzearing
reviewed
May 27, 2020
packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.base.tsx
Show resolved
Hide resolved
|
Gentle ping that this issue needs attention. |
ThomasMichon
force-pushed
the
details-list-hooks
branch
from
6bfe35b
to
aad256f
Compare
|
@dzearing and @KevinTCoughlin can you take a look at this PR? |
|
Very belatedly circling back on this--one of our major goals for version 8 is strict mode compliance, and the UNSAFE_ methods in DetailsList are one of the blockers (see #14292). We were leaning towards getting rid of the unsafe methods without conversion to a function component, but David reminded me that this PR existed. @ThomasMichon Would you have time to finish this in the next 6 weeks or so, or will someone else (likely @MLoughry or I) need to help? I also wonder if it should be re-targeted to react-next to reduce regression risk within v7. |
ThomasMichon
force-pushed
the
details-list-hooks
branch
from
aad256f
to
b42c410
Compare
|
There are a number of errors: |
ThomasMichon
force-pushed
the
details-list-hooks
branch
from
b42c410
to
b030610
Compare
|
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 b7c4f55:
|
|
Looks like they're only lint errors, the stuff on the page seems to be working correctly. |
|
For the react-hooks lint rules, it's best if you can get rid of the warning without disabling the rule, such as by fixing any real issues (definitely always take time to at least consider whether the "missing" dependencies listed could be capturing a stale mutable value), adding useCallback etc, or adding dependencies even if you know they will never change. That helps ensure future modifications to the code are also validated. If the omission is intentional, disabling the rule is fine (preferably with a comment explaining why). ESLint syntax for inline comments on disables is |
|
As far as I can tell this is ready to go in whenever the lint has been fixed. I'm approving! |
joschect
approved these changes
Aug 21, 2020
|
@dzearing @joschect @paulgildea before we merge this we should send the FYI to partners (via e-mail or teams). I don't think we need to wait a long time, just so they know a build soon will have the change (we should probably reply with the actual build when it's released). Also, I assume @ThomasMichon can help with making sure SharePoint is aware of the change it integrates well, since they will pick up releases the fastest. |
ThomasMichon
force-pushed
the
details-list-hooks
branch
from
b030610
to
b7c4f55
Compare
|
Triggering new build |
|
Closing and reopening a final time before push |
|
🎉 Handy links: |
hutchcodes
pushed a commit
to hutchcodes/fluentui
that referenced
this pull request
Sep 10, 2020
…t#12870) * Factor out hook-based inner logic from DetailsList * Change files
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Started the conversion process of
DetailsListto use React Hooks, which will provide a better platform for optimizingDetailsListand its excessive re-render issues.For the moment, a
DetailsListInnercomponent consumes the Props and State fromDetailsList, along with any internal callbacks and overridden props uses to manage the component instance.This preserves the complete behavior and caller compatibility of
DetailsList, while providing initial optimization usingReact.useCallbackandReact.useMemofor event handlers and other render props.Fixes #14292
Microsoft Reviewers: Open in CodeFlow