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

fix(pam): Fix supported UI layout for the QR Code #395

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

denisonbarbosa
Copy link
Member

The additional code entry in the layout must be optional, not required. Otherwise, this invalidates QR Codes with no code associated with them.

The additional code entry in the layout must be optional, not required.
Otherwise this invalidates QR Codes that do not have any code associated
with them.
@denisonbarbosa denisonbarbosa changed the title fix(pam): Fix supported layout for the QR Code fix(pam): Fix supported UI layout for the QR Code Jun 26, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.08%. Comparing base (fa868ac) to head (f3ceeab).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #395      +/-   ##
==========================================
- Coverage   85.24%   85.08%   -0.17%     
==========================================
  Files          76       76              
  Lines        6161     6161              
  Branches       75       75              
==========================================
- Hits         5252     5242      -10     
- Misses        644      649       +5     
- Partials      265      270       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@denisonbarbosa denisonbarbosa marked this pull request as ready for review June 26, 2024 12:43
@denisonbarbosa denisonbarbosa requested a review from a team as a code owner June 26, 2024 12:43
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

As we initially agreed!

@3v1n0
Copy link
Collaborator

3v1n0 commented Jun 26, 2024

I agree, it wasn't spotted in #387 probably because there it was made optional in the commit I had later removed which made things optional depending on a parameter (for testing purposes).

However, I think we need to check on validation because using this with the old broker it just worked...

@3v1n0
Copy link
Collaborator

3v1n0 commented Jun 26, 2024

Just a nit, in qrcodemodel you can probably avoid rendering at all if unset, but I guess bubbletea does it for us anyways.

@denisonbarbosa denisonbarbosa merged commit ca9b3a0 into main Jun 26, 2024
6 checks passed
@denisonbarbosa denisonbarbosa deleted the fix-qrcode-layout branch June 26, 2024 13:34
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.

4 participants