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: Take photos and videos and record voice memos #961

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

KhaledNjim
Copy link
Collaborator

@KhaledNjim KhaledNjim commented Jun 10, 2024

User story

Demo (android 34)

34.webm

Demo (android 33)

Screen_recording_20240603_211703.webm

Demo (android 28)

28.webm

Demo (android 27)

27.webm

Camera permission demo

permission_cam.webm

Audio recording permission demo

recper.webm

Open settings when permission is permantly denied

openSettings.webm

@KhaledNjim KhaledNjim requested a review from hoangdat June 10, 2024 04:49
@hoangdat
Copy link
Member

What is 1267? Can you describe about this issue in ADR or in github ticket, and reference commits to it?

@hoangdat
Copy link
Member

hoangdat commented Jun 10, 2024

  • demo for handling permission case also
  • demo for iOS too

ios/Runner/Info.plist Outdated Show resolved Hide resolved
ios/Runner/Info.plist Outdated Show resolved Hide resolved
@KhaledNjim
Copy link
Collaborator Author

  • demo for handling permission case also
  • demo for iOS too

idont have mac for ios demo

@KhaledNjim
Copy link
Collaborator Author

What is 1267? Can you describe about this issue in ADR or in github ticket, and reference commits to it?

added adr

@hoangdat
Copy link
Member

what happen if you reject permission?

pubspec.yaml Outdated Show resolved Hide resolved
Comment on lines 67 to 69
store.dispatch(PauseRecording());
stopwatch.stop();
audioRecorder.pauseRecording();
Copy link
Member

Choose a reason for hiding this comment

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

idem

Copy link
Member

Choose a reason for hiding this comment

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

a lot of format make the review hard

@hoangdat
Copy link
Member

hoangdat commented Jun 12, 2024

please use git fixup, dont use force push. It takes a lot of time to review the old one.
I will refuse to continue if you force push on this big changes.

@KhaledNjim
Copy link
Collaborator Author

what happen if you reject permission?

display a toast and return to the previous screen

@KhaledNjim KhaledNjim requested a review from hoangdat June 26, 2024 10:15
"placeholders_order": [],
"placeholders": {}
},
"give_access_in_settings": "Please give linshare access in the settings to continue",
Copy link
Member

Choose a reason for hiding this comment

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

LinShare, Settings

Copy link
Member

Choose a reason for hiding this comment

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

And we should more detail on what permission and why we need it? This is a chance for clearer information to user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

image

child: Text(AppLocalizations.of(context).cancel),
),
TextButton(
onPressed: () => Navigator.of(context).pop(true),
Copy link
Member

Choose a reason for hiding this comment

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

try to handle the case: user cancel here, and somewhere after capturing video/audio call, we should clear the temporary file too. Please demo this case also

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Handled

cache_cleanup.mp4

"placeholders_order": [],
"placeholders": {}
},
"exit": "exit",
Copy link
Member

Choose a reason for hiding this comment

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

Discard

Copy link
Member

Choose a reason for hiding this comment

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

@KhaledNjim please dont forget it

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.

3 participants