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.

Sign up for GitHub

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

Jump to bottom

feat: Add SafeArea to ScannerOverlay page #1130

Merged
merged 2 commits into from
Feb 12, 2022
Merged

feat: Add SafeArea to ScannerOverlay page #1130

merged 2 commits into from
Feb 12, 2022

Conversation

danagbemava
Copy link
Contributor

What

  • This PR adds a SafeArea to the buttons on the ScannerOverlay page to prevent them from appearing under the status bar.

Screenshot

before after
photo_2022-02-12 11 06 18 photo_2022-02-12 11 06 22

Fixes bug(s)

Part of

@danagbemava danagbemava requested a review from a team as a code owner February 12, 2022 13:06
monsieurtanuki
monsieurtanuki requested changes Feb 12, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

The SafeArea should rather be on the top element (in that case the Container), shouldn't it? Maybe even on top of the LayoutBuilder.
Would you please test if the screenSize is the same if you put your LayoutBuilder as a child of SafeArea?

@M123-dev
Copy link
Member

Why not putting it around there when it works, as long as no other things get cropped away. When putting it around everything it will again also crop the CameraPreview

@monsieurtanuki
Copy link
Contributor

@M123-dev I don't agree, but not to the point of preventing the PR from passing.

@monsieurtanuki monsieurtanuki self-requested a review February 12, 2022 13:38
monsieurtanuki
monsieurtanuki reviewed Feb 12, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Not a big fan of full screen, but some people think differently :)

monsieurtanuki
monsieurtanuki approved these changes Feb 12, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Apparently I need to approve in order to cancel my "request changes".

@danagbemava
Copy link
Contributor Author

danagbemava commented Feb 12, 2022 •
edited
Loading

The SafeArea should rather be on the top element (in that case the Container), shouldn't it?

That isn't always the case. I got the idea for this from how the MaterialBanner avoids the status bar. It is wrapped in a SafeArea of its own so that it can avoid the status bar when the page it's on doesn't have an AppBar.

@teolemon teolemon added the 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… label Feb 12, 2022
@M123-dev
Copy link
Member

Lets check how it looks and we can always change things later

@M123-dev M123-dev merged commit efdba9c into openfoodfacts:develop Feb 12, 2022
@M123-dev
Copy link
Member

Merged @danagbemava 👍🏼

@github-actions github-actions bot mentioned this pull request Feb 12, 2022
Closed
@danagbemava danagbemava deleted the safearea-fix branch February 12, 2022 17:11
@M123-dev M123-dev mentioned this pull request Feb 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monsieurtanuki monsieurtanuki monsieurtanuki approved these changes

Assignees
No one assigned
Labels
🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@danagbemava @M123-dev @monsieurtanuki @teolemon