Skip to content
This repository has been archived by the owner on Jan 6, 2025. It is now read-only.

feat(capture): check if camera is available before trying to capture #20

Merged

Conversation

Thithip
Copy link
Contributor

@Thithip Thithip commented Mar 26, 2024

On some devices (desktop, web, iOS emulators) camera can be unavailable, and the app display a verbose error.
With this feature, we display an error for the User, and the way to return the Home page (thanks to the Scaffold).

Plus, I fix the scroll on Home page (by removing the hard-coded SizedBox), to allow scrolling on every devices, no matter the size of it.
And I apply some lint issues (const, brackets, misspelling word).

I can split this MR is multiple if you consider this one is too big, or if you think some changes are not mandatory.
Please let me know if you want I add/remove some stuff here ;)

@luifr10
Copy link
Contributor

luifr10 commented Mar 28, 2024

Hello @Thithip thanks for the MR
Yes I think it is a good idea to split this MR in two to separate control of camera presence and scrolling fix on the home page

@Thithip
Copy link
Contributor Author

Thithip commented Mar 28, 2024

Ok, I'll do this later.
Do you think I should split it in multiple commits (one per feature or fix), or into multiple MR?

Thanks for review 👍

@Thithip Thithip force-pushed the fix/check_is_camera_are_available branch 2 times, most recently from 040eb49 to 22a6f9f Compare March 29, 2024 09:09
@Thithip
Copy link
Contributor Author

Thithip commented Mar 29, 2024

@luifr10 I split this MR into 4 commits, to add clarity on it.

Tell me if it's ok for you. I can split into multiple MR if you think it makes more sense.

Copy link
Contributor

@luifr10 luifr10 left a comment

Choose a reason for hiding this comment

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

Thanks again for your MR, I just have a minor feedback

return Scaffold(
appBar: AppBar(),
body: const Center(
child: Text('No camera found for this device'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the i10n mechanism:

  1. First add a new key (newAddedKey) to the lib/l10n/app_en.arb and lib/l10n/app_fr.arb files.
  2. Then use it in dart file : AppLocalizations.of(context)!.newAddedKey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, nice catch!
I forgot this, sorry.

Thithip added 4 commits April 2, 2024 09:43
On some devices (desktop, web, iOS emulators) camera can be unavailable, and the app display a verbose error.
With this feature, we display an error for the User, and the way to return the Home page (thanks to the Scaffold).
- remove hard-coded SizedBox
- add a more dynamic way to scroll over Sequences, no matter the device size
- no more cover the "Your sequences" title list
On local, Android behavior differ from others platforms with a specific `10.0.2.2` address.
Adding a condition on this allows others devices (such as iOS, MacOS, or desktop) to connect to the local API for tests
- apply lint
- add const when needed
- fix misspelling issue
- remove unnecessary `new` keyword
@Thithip Thithip force-pushed the fix/check_is_camera_are_available branch from 22a6f9f to f17b41a Compare April 2, 2024 07:44
@luifr10 luifr10 merged commit 1174746 into nobelization:main Apr 3, 2024
2 checks passed
@Thithip Thithip deleted the fix/check_is_camera_are_available branch April 9, 2024 09:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants