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

Use FlatList instead of our custom recycler view list on Android #256

Merged
merged 3 commits into from Nov 22, 2018

Conversation

daniloercoli
Copy link
Contributor

This PR switches the main blocks list to ReactNative FlatList component on Android.

  • The switch back to FlatList was required since there are issues selecting text in any of the blocks in the list. The issue is not limited to AztecWrapper powered blocks, such as Para and Heading, but it's affecting the code block that is backed by TextInput.
  • There are also problems with the contextual menu that does appear on the screen. There is no wat to paste or copy text at the moment.

I've spent some time trying to fix the issue in our recycler view list repo, with no luck.

It's not clear to me why we've selected this custom list component (beside animations), since FlatList renders items lazily, just when they are about to appear, and removes items that scroll way off screen to save memory and processing time.

If we all agree on removing the Recycler list only temporary, hoping to find the time to fix the issue later, I can open a new ticket to remind us to remove all the Datasource refs required by this component, if the fix on the list doesn't' happen in say 2 weeks, or a month.
Otherwise I can remove all refs in this PR.

@Tug
Copy link
Contributor

Tug commented Nov 20, 2018

I suggest we remove the whole DataSource part, we would have had to refactor it anyway.

Imo, it will be simpler to find a proper solution starting from scratch if we switch back

Edit: I added a commit to do just that, feel free to remove it if you don't want it here!

@koke
Copy link
Member

koke commented Nov 21, 2018

I just tested this PR and I think inserting and reordering in FlatList seems more natural than with the previous animations, so if that's the only thing keeping us on the recyclerview one, I'm happy to drop it

@mzorz
Copy link
Contributor

mzorz commented Nov 21, 2018

Regarding using FlatList or RecyclerView on Android, we've been chatting with @daniloercoli about it. As we approach the alpha release date, I think it's good to have this FlatList PR ready to merge if we see we are out of time by the date Alpha needs to be sent out. It's good to have alternatives.

Clearly, the focus problems make Gutenberg currently hardly usable on Android, and we need to fix them, but just testing this PR on a real device shows me the focus problems are still there for Aztec (RichText). So, it doesn't look like it's the RecyclerView's fault.

It's not clear to me why we've selected this custom list component (beside animations), since FlatList renders items lazily, just when they are about to appear, and removes items that scroll way off screen to save memory and processing time.

Regarding the choice itself, I'm not super clear about history but leaving what I recall and my thoughts here to document from this point on. I think we were concerned about these things when the initial decision was made:

  • performance (FlatList was supposed to be a complete list, not a Recycling list at that time)
  • animations (again, performance) natively
  • native support for drag and drop (seeing convoluted libraries doing calculations in javascript was something we were concerned about)
  • we as a team had more expertise / knowledge in native at that time, than we have now

I think it'd be good to make a list/matrix with pros and cons for each. I can see code using FlatList being cleaner and more maintainable, as opposed to using the RecyclerViewList which sometimes is turning rather hackish and patchy as we move (I have to clarify here the recent focus patch it's not an intrinsic problem of the RecyclerView itself but rather due to a bug in RN's InputText, that becomes apparent when using the RecyclerView). OTOH we are supposed to be gaining in performance by having the native Android RecyclerView in place, native animations, and drag and drop should be easy to implement natively.
But for native animations to work, we have to have JS code that manipulates the dataSource directly, to signal the RecyclerView a movement needs to happen.

Pros of using FlatList:

  • standard React Native
  • easy to implement
  • code is cleaner
  • it's being used for iOS already, so we get a more consistent codebase across both platforms by using the same component.

Cons of FlatList:

  • not having enough control over it (how it works or changes to it might impact performance or our ability to fix things)
  • if we do animations, we'll have to do that with JS (might have performance implications / unknowns), find a library or build our own
  • if we want drag and drop, we'll have to do that with JS (same as above - find a library or build our own)

Pros of using RecyclerViewList:

  • android native, unbeatable performance
  • easy to implement for us (team expertise)
  • we already have animations natively
  • drag and drop is easy to implement natively

Cons of using RecyclerViewList:

  • divergent codebase (from iOS)
  • needs to be maintained (I'm not sure whether it is an intrinsic RecyclerViewList problem? as other third party libraries, even if JS-based will have this problem too)
  • code in block-manager is not as clean: needs code to handle dataSource directly for animations to work (and the code model diverges from the React/Redux pattern as things are manipulated directly without waiting for the React component lifecycle to update state, etc.)
  • focus problems. Honestly these are hardly due to the RecyclerView itself. Most of the problems are about Aztec and how RN handles focus for entering text: we will have these problems inevitably because React Native controls focus "knowing" or keeping a state for all InputText instances, and we have Aztec's focus which RN knows nothing about, at least not yet, we need to work on that soon)

Interested in knowing your thoughts cc @hypest @koke @daniloercoli and everyone

@koke
Copy link
Member

koke commented Nov 22, 2018

Wow, thanks @mzorz for all the context and the thorough comparison ❤️

Some comments on the main issues:

  • Animations: as mentioned before, we don't need the animations for what we have right now, the UX feels more natural without them.
  • Drag and drop: we're not worrying about that for at least 3-4 months, we can evaluate our options when we look into it.
  • Not having enough control over FlatList: React Native is open source, and if we need to, we can fork [FlatList](not having enough control over) as well.
  • Performance: as I understand it we have an intuition that RecyclerView will be more performant, but as far as I know, we haven't seen any performance issues yet, is that correct?

To add to the list, I'm not 100% sure about the details, but I feel we've had to spend a good amount of effort adapting to the recyclerview list, and fixing issues with how the dataSource works, and I'm under the impression that working with FlatList would have been more straightforward.

With all this information, I'm strongly in favor of switching to FlatList.

@daniloercoli
Copy link
Contributor Author

daniloercoli commented Nov 22, 2018

Thanks for putting our brainstorming in a complete form. 🥇

To add to the list, I'm not 100% sure about the details, but I feel we've had to spend a good amount of effort adapting to the recyclerview list, and fixing issues with how the dataSource works, and I'm under the impression that working with FlatList would have been more straightforward.

With all this information, I'm strongly in favor of switching to FlatList.

I agree with @koke on this specific point above, and also on the others points.
Also, as he already said, we can re-introduce recycler view in 3-4 months if required.

We should also think that RN isn't a new framework in 2018, there should be reasons behind their choices of using what it seems a pure JS approach for Virtual Lists. I would be surprised that RN doesn't handle virtual lists in a reasonable way, since it's one of the most used component out there in apps.

@Tug Tug force-pushed the fix/remove-RecyclerViewList-Android branch from f63e50a to ea6cfd8 Compare November 22, 2018 09:19
@Tug Tug force-pushed the fix/remove-RecyclerViewList-Android branch from ea6cfd8 to 6562e23 Compare November 22, 2018 09:43
@Tug
Copy link
Contributor

Tug commented Nov 22, 2018

Rebased (git pull -r to pull the last changes)

@daniloercoli
Copy link
Contributor Author

Tested this again and it seems to work as expected.
(if you test this on a real device, there are still those known focus problems on blocks that use AztecWrapper under the hood).

@mzorz
Copy link
Contributor

mzorz commented Nov 22, 2018

We should also think that RN isn't a new framework in 2018, there should be reasons behind their choices of using what it seems a pure JS approach for Virtual Lists. I would be surprised that RN doesn't handle virtual lists in a reasonable way, since it's one of the most used component out there in apps.

Agreed - and we should be able to use any other RN lib to add animations and drag and drop to both platforms this way. To me the strongest point is cleaner code and maintainability. I'm leaning towards FlatList as well 👍

Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

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

we can re-introduce recycler view in 3-4 months if required.

As discussed elsewhere, concerns for people against this PR are that FlatList won't be able to give us what we need for a good, "native feel" UX.
However, and in lieu of keeping things simple for development to hit our goals for Alpha, the team has the feeling FlatList will help have things out of the way and focus on isolating the problems ahead.

Approving this PR to get things going, knowing we'll need to come back to RecyclerViewList later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants