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 configuration screen #87

Open
TimoGlastra opened this issue Aug 31, 2022 · 4 comments · Fixed by #109
Open

Add configuration screen #87

TimoGlastra opened this issue Aug 31, 2022 · 4 comments · Fixed by #109
Labels
enhancement New feature or request

Comments

@TimoGlastra
Copy link
Member

TimoGlastra commented Aug 31, 2022

It would be nice to add a configuration screen (accessed by cmd+k) that allows to configure which features to use for the demo. This way you can use the demo with some of the new feautres, without breaking interop with old wallets. Kind of like flags in your browser (very hidden, but possible to change if you know what you're doing).

The features I'm currently thinking of:

  • ledger
  • issue credential protocol version
  • present proof protocol version
  • oob vs legacy invitation

We can add custom configuration to the cmd/ctrl+k screen like we already have with change theme (see screenshot)

image

@janrtvld
Copy link
Contributor

janrtvld commented Oct 6, 2022

Should we keep this open for the other 2 features or wait for them to be implemented in AFJ?

@TimoGlastra
Copy link
Member Author

Maybe create separate issues for it?

I did notice some issues with the current implementation:

  • If I do reset demo with configuration options, the theme is glitching (see attached video)
  • I can't see the currently selected item (credential protocol version or connection type) (see other attached video)
  • There is now both configuration and preferences, which to me are two different words for the same thing. I was thinking what if we remove the configuration, and add all the new settings to the top level config screen? There's quite some nesting going on, which isn't needed I think (see attached screenshot)
  • after thinking about it a bit more, I do think we should split reset demo (to reset the demo) and reset configuration (to reset everything in the configuration section of the settings)
Screen.Recording.2022-10-06.at.15.35.36.mov

Screen Shot 2022-10-06 at 15 37 20

Screen.Recording.2022-10-06.at.15.35.14.mov

@janrtvld @rapaktech what do you think?

@janrtvld
Copy link
Contributor

janrtvld commented Oct 6, 2022

  • If I do reset demo with configuration options, the theme is glitching (see attached video)

Fixed that one.

  • I can't see the currently selected item (credential protocol version or connection type) (see other attached video)

Jim and i talked about it and we just totally forgot about it haha.

  • There is now both configuration and preferences, which to me are two different words for the same thing. I was thinking what if we remove the configuration, and add all the new settings to the top level config screen? There's quite some nesting going on, which isn't needed I think (see attached screenshot)

Great solution. I think we can assume that most people that find out about the cmd+k menu know what they are doing so all these options wouldn't really confuse them.

  • after thinking about it a bit more, I do think we should split reset demo (to reset the demo) and reset configuration (to reset everything in the configuration section of the settings)

This option seemed the best as i assumed it would result in issues if you'd let the user change settings halfway through the demo. Now that we've implemented it i noticed this works just fine so i like this change.

Are you able to pick these up, @rapaktech? I can help you with the currently selected option as that might be a bit more tricky but the other two should be pretty easy.

@jimezesinachi
Copy link
Contributor

jimezesinachi commented Oct 6, 2022

Quick question. We already have resetState that is basically resetDemo - resetConfig, and resetDemo that is resetState + resetConfig. So now I should rewrite resetDemo to something that essentially is resetConfig, correct?

@TimoGlastra @janrtvld

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants