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

feat(capture-sdk) Add inform "qr code not available" message #547

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

GeraltRiv
Copy link
Contributor

PP-143

@GeraltRiv GeraltRiv requested a review from a-szotyori August 23, 2024 12:48
Copy link
Contributor

@a-szotyori a-szotyori left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few small requests regarding text localisation and tablet layouts.

Copy link
Contributor

@a-szotyori a-szotyori left a comment

Choose a reason for hiding this comment

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

Sorry for requesting more changes, but I noticed some more UI differences:

  • "Body2" font style is not used
  • positioning on tablets is not aligned with the bottom of the frame (white corners)
  • inner padding should be 8dp on phones and 16dp on tablets (you can view the distances in Figma with the option (⌥) button)
  • drop shadow is missing (if not possible or too complicated to show it back to API Level 21, then only show from a more recent API level)

I made these screenshots to compare design (left) and implementation (right):
Screenshot 2024-08-27 at 14 04 56
Screenshot 2024-08-27 at 14 05 49
Screenshot 2024-08-27 at 14 07 25

android:background="@drawable/gc_detection_error_background"
xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto">
<TextView
Copy link
Contributor

Choose a reason for hiding this comment

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

[Request] Please use style="@style/GiniCaptureTheme.Typography.Body2" for the text.

android:text="@string/gc_detection_error_layout_description"/>


<TextView
Copy link
Contributor

Choose a reason for hiding this comment

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

[Request] Also here please use style="@style/GiniCaptureTheme.Typography.Body2".

Copy link

sonarcloud bot commented Aug 27, 2024

Copy link
Contributor

@a-szotyori a-szotyori left a comment

Choose a reason for hiding this comment

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

Looks great now! Thank you! Can go to QA 🎉

@GeraltRiv GeraltRiv merged commit 2834f67 into main Sep 9, 2024
17 checks passed
@GeraltRiv GeraltRiv deleted the PP-143_QR_code_error_layout branch September 9, 2024 09:50
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