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

Human readable QR #678

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Human readable QR #678

wants to merge 11 commits into from

Conversation

Gavinok
Copy link
Contributor

@Gavinok Gavinok commented Nov 8, 2024

This pr resolves #646

The key changes is the addition of the new metadata entry in the VerificationConfig that can contain the title of the verification and the claims it verifies. Here an example of the UI change on mobile.

2024-11-08_10-36-37

and for desktop

2024-11-08_10-42-40

Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok Gavinok requested review from loneil and esune November 8, 2024 18:53
Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

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

I think this should work well!

The only thing is that - at least to me - the alignment of the new paragraph seems to be off when compared with the rest of the page (which seems to be more centered). Is it true or are my eyes tricking me into seeing that?

@loneil
Copy link
Contributor

loneil commented Nov 8, 2024

I think this should work well!

The only thing is that - at least to me - the alignment of the new paragraph seems to be off when compared with the rest of the page (which seems to be more centered). Is it true or are my eyes tricking me into seeing that?

This is the alignment, so paragraph left justified. Think center justifying it would then have multiple list items looking weird, so if that were the case think it would be centering just the paragraph (but then having to offset each list item so they line up)

image

@esune
Copy link
Member

esune commented Nov 12, 2024

I think this should work well!
The only thing is that - at least to me - the alignment of the new paragraph seems to be off when compared with the rest of the page (which seems to be more centered). Is it true or are my eyes tricking me into seeing that?

This is the alignment, so paragraph left justified. Think center justifying it would then have multiple list items looking weird, so if that were the case think it would be centering just the paragraph (but then having to offset each list item so they line up)

image

Yeah, center-justifying is not the way to go. It looked like things "leaned" to the left like this, but if the box space being occupied is what is already being used then let's keep it as-is.

Gavinok and others added 2 commits November 12, 2024 10:23
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok Gavinok requested a review from loneil November 12, 2024 23:07
loneil
loneil previously approved these changes Nov 13, 2024
Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

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

Think this should do the trick 👍

esune
esune previously approved these changes Nov 13, 2024
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok Gavinok dismissed stale reviews from esune and loneil via 05022ca November 15, 2024 00:07
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok Gavinok requested review from loneil and esune November 15, 2024 00:15
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
esune
esune previously approved these changes Nov 18, 2024
Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

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

I think this will work 👍🏻

@loneil
Copy link
Contributor

loneil commented Nov 18, 2024

Are we just adding a language array to the claims and not the title?

@esune
Copy link
Member

esune commented Nov 18, 2024

Are we just adding a language array to the claims and not the title?

I somehow missed that, good catch. We should add the same language mapping to the title - maybe we should have the language map as the root element with language and title both nested within it @Gavinok

Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@Gavinok
Copy link
Contributor Author

Gavinok commented Nov 18, 2024

Added support for titles thanks for pointing that out. Should have just added that to begin with.

@Gavinok Gavinok requested a review from esune November 18, 2024 20:42
Comment on lines 1344 to 1352
metadata={
"title": {"en": "Get Name"},
"claims": {
"en": [
"That you are a BC Resident",
"That you are a member of the Law Society of British Columbia",
]
},
},
Copy link
Member

Choose a reason for hiding this comment

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

What about using a single language key for each entry, rather than duplicating it?

Suggested change
metadata={
"title": {"en": "Get Name"},
"claims": {
"en": [
"That you are a BC Resident",
"That you are a member of the Law Society of British Columbia",
]
},
},
metadata={
"en":{
"title": "Get Name",
"claims": [
"That you are a BC Resident",
"That you are a member of the Law Society of British Columbia",
]
}
},

@@ -212,6 +212,20 @@ async def get_authorize(request: Request, db: Database = Depends(get_db)):
"controller_host": controller_host,
"challenge_poll_uri": ChallengePollUri,
"wallet_deep_link": wallet_deep_link,
"title": (
ver_config.metadata.title.get("en")
Copy link
Member

Choose a reason for hiding this comment

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

It might be best to use a parameter for the language, so that we don;t have to update this condition when parsing a language dynamically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like a parameter for the function or for the template?

Copy link
Member

Choose a reason for hiding this comment

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

I think what you did will work, once we have multilanguage support in the UI we just need to update the code to grab that value rathe rthan the hardcoded en that now only appears in one spot

…claims

Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

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

Looks good, tried a couple permutations out locally

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.

Add human-readable proof-request information to QR code page
3 participants