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

@swm/add draggable list #7

Closed
wants to merge 49 commits into from

Conversation

kosmydel
Copy link

@kosmydel kosmydel commented Aug 24, 2023

Details

This PR adds support for both draggable lists on both web (using react-beautiful-dnd and native (using react-native-draggable-flatlist).

Related Issues

Expensify/App#22716
Expensify/App#25740

Manual Tests

Linked PRs

@github-actions
Copy link
Contributor

github-actions bot commented Aug 24, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

.prettierrrc.js Outdated Show resolved Hide resolved
LICENSE Outdated Show resolved Hide resolved
example/src/App.tsx Outdated Show resolved Hide resolved
example/src/utils.tsx Outdated Show resolved Hide resolved
@hayata-suenaga hayata-suenaga self-requested a review August 28, 2023 14:38
@kosmydel kosmydel marked this pull request as ready for review August 28, 2023 14:51
@hayata-suenaga
Copy link
Contributor

@kosmydel Today, Expensify is internally decided not to create additional libraries if not needed.

How much functionality did you implement (or rather exposed) in this library? If we're just exposing just the functionality used in NewDot, then it might be better to move the code over to NewDot to make things easier.

Let me know what you think 🙇

@kosmydel
Copy link
Author

@kosmydel Today, Expensify is internally decided not to create additional libraries if not needed.

How much functionality did you implement (or rather exposed) in this library? If we're just exposing just the functionality used in NewDot, then it might be better to move the code over to NewDot to make things easier.

Let me know what you think 🙇

@hayata-suenaga
I understand, here are my thoughts:

My implementation:
Since my approach was to use the react-native-draggable-flatlist API in the new library, on the native the code was short. The more code was implemented on the web side, to make the react-beautiful-dnd work with the native API. Because there were some differences, I had to adjust it a bit. I think that the source will tell more about how many things were implemented here.

I focused on exposing the functionality the NewDot app needed. There are also some extras like decorators.

Of course, we can move this code to the NewDot app. Since this was mainly the wrapper, it might work. Please let me know about the next steps.

@hayata-suenaga
Copy link
Contributor

Let's move the code over to App. I think we don't want to deal with the effort to maintain another library 🙇

There are also some extras like decorators.

could you tell me what decorators do? do we need them in App?

@kosmydel
Copy link
Author

Let's move the code over to App. I think we don't want to deal with the effort to maintain another library 🙇

Sure, I am going to prepare PR for that. As I have internal training until Friday, my availability will be limited.

There are also some extras like decorators.

could you tell me what decorators do? do we need them in App?

Those decorators work only on native, and 'decorate' items that are currently dragged (e.g. by scaling the item up a little bit or changing the opacity).
Here is more context. Those are probably not really important, but we might consider adding something like this. Moving this part of the code to the App should be easy.

@neil-marcellini
Copy link
Contributor

Closing in favor of Expensify/App#26307

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.

4 participants