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 explanation section #24

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ricklambrechts
Copy link

@ricklambrechts ricklambrechts commented Oct 31, 2023

Basic working implementation of how we could implement an explanation section for a specific state.

Would like to receive feedback!

Yivi popup issuing flow in dutch with the changes in our application.
image

After scanning the QR code with the Yivi app, the explanation is gone. I think the explanation is also not needed because of the message that is visible. But it is possible to add another explanation based on the state in the language files.
image

Currently the height is fixed. We need to update that to support this additional section.

// Acts as min-height, but IE11 needs a fixed
// parent height for the vertical centering.
height: $qr-code-size + 224px;

@MikeClickr
Copy link

Well done @ricklambrechts ! I oppose to use the following text for mobil use:
Gebruik uw Yivi-app om uw gegevens door te geven.
Yivi Connect - Desktop
Yivi Connect - Mobiel

@ricklambrechts
Copy link
Author

When you are on mobile you see state 3b.

image

@bobhageman
Copy link
Contributor

bobhageman commented Oct 31, 2023

Very nice improvement! One thing: I would like to keep the translation files as plain as possible. Thereby I suggest to move the explanation: function(... logic out of nl.js and en.js.

And it might also be fine to enable the explanation by default (showExplanation=true) but that's something we will discuss first.

@bobhageman
Copy link
Contributor

@ricklambrechts, we've decided the explanation should not be enabled by default to maximize backwards compatibility. Therefore you can leave that setting as is;)

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.

3 participants