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

Feature swap file #42

Closed
wants to merge 4 commits into from
Closed

Feature swap file #42

wants to merge 4 commits into from

Conversation

axel-legue
Copy link

It's my first contribution to an Open Source Project,
So let me know if i do something wrong.

axel legue added 4 commits January 29, 2019 02:48
 - Change the way we get the recordings items in RecyclerView
 - Change DatabaseListener for new entries from recyclerView to Fragment.
 - Add Multiple interface for touch event
 - Create a custom ItemTouchHelperCallback for handle drag and drop

 Issues #23
 - Update Rename item listener
 - Update the ReecyclerView properly when item renamed or deleted
 - Add code
 - Clean some commented code
@naXa777 naXa777 self-requested a review January 30, 2019 15:50
@naXa777
Copy link
Owner

naXa777 commented Jan 30, 2019

Thank you for your contribution! I'll review it in the next few days.

  1. don't worry about the failing Travis CI build. It's just broken for pull requests from external repositories.
  2. you asked for feedback, so... I noticed that you mentioned the issue Drag and drop to rearrange items in Stored Recordings list #23 in a commit message. and you could also refer this issue in PR description, like "fixes Drag and drop to rearrange items in Stored Recordings list #23", for convenient navigation. anyway, it's not a big deal =)

@naXa777
Copy link
Owner

naXa777 commented Feb 14, 2019

@axel-legue you've implemented records reordering and it's working fine.

After manual testing I discovered the following issues:

  • Crashlytics initialization fails with the following exception: "UnmetDependencyException The Crashlytics build ID is missing. This occurs when Crashlytics tooling is absent from your app's build configuration. Please review Crashlytics onboarding instructions and ensure you have a valid Crashlytics account."
  • User can't listen to records. In this app, user can go to "saved recordings" tab and tap on any item in list to open a Media Player (see PlaybackFragment). But now it doesn't happen.
  • Travis CI build fails.

Possible solutions:

  • don't change Crashlytics initialization code! revert Fabric.with(this, new Crashlytics()); to Fabric.with(fabric);
  • I suggest adding a drag handler for each item. A user can reorder items by holding this handler, and a user can open recordings for listening by tapping on items. Refer to https://material.io/design/components/lists.html#behavior for example (see Behaviour > Gestures).
  • ignore Travis CI build for this Pull Request. if your local build fails due to Crashlytics, just add fabric.properties file with apiSecret and apiKey to the project root directory. DO NOT commit this file to Github.

Please fix the first two issues and then I review this PR again.

Copy link
Owner

@naXa777 naXa777 left a comment

Choose a reason for hiding this comment

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

fix the above suggestions or explain why not

@naXa777
Copy link
Owner

naXa777 commented Oct 23, 2022

closing stale PR

@naXa777 naXa777 closed this Oct 23, 2022
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.

2 participants