-
Notifications
You must be signed in to change notification settings - Fork 0
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 details view #108
base: main
Are you sure you want to change the base?
Add details view #108
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
=======================================
Coverage 12.04% 12.04%
=======================================
Files 6 6
Lines 83 83
=======================================
Hits 10 10
Misses 73 73 Continue to review full report in Codecov by Sentry.
|
f292ace
to
32ff24e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to tweak the developer bit and then we can pull this in.
const showDisplays = ref(false); | ||
const showHighlights = ref(false); | ||
const credentialImages = reactive([]); | ||
const showDetails = ref(process.env.NODE_ENV === 'development'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code runs in the browser, so we can't use node.js environment variables for this. We probably just need it to say "(Developer Only)" or something for now, so that regular users don't assume it's safe to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const showDisplays = ref(false); | ||
const showHighlights = ref(false); | ||
const credentialImages = reactive([]); | ||
const showDetails = ref(config?.vueWallet?.developmentMode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've been unclear about this particular feature. What do you think about having this always be enabled (no configuration for it), but clearly marking it as a "Developer feature" or with a "Developer only warning" in the UI / title / display area (above the tree) somewhere?
Another option would be to enable it via a menu or something -- but I don't think we want to maintain and deal with any static / deployment configuration for this -- and it will be useful to devs in production wallets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great - I'll add the warning text and remove the configuration. Thanks for the feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed configuration and added warning banner: d36c512
d2d1015
to
f62d23c
Compare
Resolves - add details view for credential details window
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
Does this PR introduce a breaking change?
How has this been tested?
Screenshots:
n/a