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

Add second consent message language and make message more descriptive #2073

Merged
merged 14 commits into from
Nov 27, 2023

Conversation

frederikrothenberger
Copy link
Contributor

@frederikrothenberger frederikrothenberger commented Nov 23, 2023

This PR improves the consent messages to be available in 2 languages. The message is improved to be more human readable and is now formatted as markdown.

Additionally, the ICRC21 types were updated to reflect the recent spec changes. Adjusting to the actual types with came case will be done in a separate PR because the issuer API needs adjustment too.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
This PR improves the consent messages to be available in 2 languages.
The message is improved to be more human readable and is now formatted
as markdown.

Additionally, the ICRC21 types were updated to reflect the recent
spec changes.
@nmattia
Copy link
Collaborator

nmattia commented Nov 23, 2023

The message is improved to be more human readable and is now formatted as markdown.

Does that mean the frontend should render the message as markdown formatted?

@frederikrothenberger
Copy link
Contributor Author

The message is improved to be more human readable and is now formatted as markdown.

Does that mean the frontend should render the message as markdown formatted?

Yes, ICRC-21 specifies markdown formatting.

Copy link
Collaborator

@nmattia nmattia left a comment

Choose a reason for hiding this comment

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

Couple of suggestions but LGTM nonetheless!

demos/vc_issuer/src/consent_message.rs Outdated Show resolved Hide resolved
demos/vc_issuer/src/consent_message.rs Outdated Show resolved Hide resolved
demos/vc_issuer/src/main.rs Outdated Show resolved Hide resolved
demos/vc_issuer/tests/issue_credential.rs Outdated Show resolved Hide resolved
demos/vc_issuer/tests/issue_credential.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@przydatek przydatek left a comment

Choose a reason for hiding this comment

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

Thanks @frederikrothenberger!
Please see some suggestions/remarks as discussed.

demos/vc_issuer/tests/issue_credential.rs Outdated Show resolved Hide resolved
demos/vc_issuer/tests/issue_credential.rs Outdated Show resolved Hide resolved
demos/vc_issuer/src/consent_message.rs Outdated Show resolved Hide resolved
demos/vc_issuer/src/consent_message.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@przydatek przydatek left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the updates @frederikrothenberger!
I have one more question, and one minor suggestion, PTAL.

src/vc_util/src/issuer_api.rs Show resolved Hide resolved
demos/vc_issuer/src/consent_message.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@przydatek przydatek left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@frederikrothenberger frederikrothenberger added this pull request to the merge queue Nov 27, 2023
Merged via the queue into main with commit fa918d2 Nov 27, 2023
53 checks passed
@frederikrothenberger frederikrothenberger deleted the frederik/consent-msg-improvement branch November 27, 2023 08:48
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.

None yet

3 participants