Uh oh!
There was an error while loading. Please reload this page.
Convert network overlay to use flatlist - #21837
Conversation
facebook-github-bot
commented
Oct 17, 2018
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
f2487aa to
d3073f5Comparefacebook-github-bot
commented
Oct 17, 2018
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
There was a problem hiding this comment.
I'm not a massive fan of the localised state mutation, but it feels like an improvement to the existing code.
I assumed the previous mutation was for performance reasons, and I've aligned with the most simplistic approach for now.
d3073f5 to
c09fc6dCompareelicwhite
commented
Oct 18, 2018
Thanks for this! I was just looking today at how this is one of the only callsites of ListView in the repo. Unfortunately, this PR is huge and makes it hard to read through and see what changes are necessary to each piece. Can you break this into a couple different changes?
Each of these changes seem like they could be done in isolation. It would be greatly appreciated if you could make it as easy as possible to review your changes. We definitely want these things to land. |
AlanFoster
commented
Oct 18, 2018
@TheSavior I agree - it's quite the PR. The code in the file was previously quite complected, but thankfully it's a single component still!
The main responsibilities are:
I can try to extract this into two separate commits, but the commits wouldn't be atomic as they wouldn't work in isolation without lots of extra glue code to help with an intermediate step, which might make the review more confusing! Let me know if the additional insights make this PR easier to review, or if you'd still like two separate commits :] |
elicwhite
commented
Oct 18, 2018
Thanks for that explanation. I'm not sure I understand why the changes to network handling logic / setState are deeply coupled to ListView/FlatList. Can you help me understand this better? It seems like it should be possible to migrate to FlatList without also migrating the business logic very much. |
AlanFoster
commented
Oct 18, 2018
@TheSavior That was my initial plan too, but once I learnt what the existing code was doing - it ended up easier to modify the code to be closer to idiomatic React. Let me know your thoughts after you've had a chance to review it 👍 |
elicwhite
commented
Oct 18, 2018
I tried to import the PR to test it manually but noticed that your target branch is |
c09fc6d to
60a222bCompareAlanFoster
commented
Oct 18, 2018
@TheSavior Done! Apologies :) |
60a222b to
375c8c2Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
TheSavior has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
react-native-bot
commented
Oct 23, 2018
@AlanFoster merged commit efa6016 into |
Summary: Pull Request resolved: react#21837 Reviewed By: TheSavior Differential Revision: D10449940 Pulled By: RSNara fbshipit-source-id: 28dcd5906c64070ef50867425ae7517d391300e2
Test Plan:
I ran through the following scenarios:
Release Notes:
Help reviewers and the release process by writing your own release notes. See below for an example.
[GENERAL] [ENHANCEMENT] [Network Inspector] - The network inspector has been updated to use FlatList, as ListView has been marked as deprecated.