-
Notifications
You must be signed in to change notification settings - Fork 100
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
Fabo/sign in msg #2151
Fabo/sign in msg #2151
Conversation
Codecov Report
@@ Coverage Diff @@
## jordan/1982-session-2.0 #2151 +/- ##
===========================================================
+ Coverage 95.22% 95.23% +0.01%
===========================================================
Files 106 107 +1
Lines 2261 2266 +5
Branches 111 111
===========================================================
+ Hits 2153 2158 +5
Misses 97 97
Partials 11 11
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## jordan/1982-session-2.0 #2151 +/- ##
===========================================================
+ Coverage 95.22% 95.23% +0.01%
===========================================================
Files 106 107 +1
Lines 2261 2266 +5
Branches 111 111
===========================================================
+ Hits 2153 2158 +5
Misses 97 97
Partials 11 11
|
@@ -17,7 +17,8 @@ const getters = { | |||
committedDelegations: { | |||
}, | |||
connected: true, | |||
bondDenom: `stake` | |||
bondDenom: `stake`, | |||
session: { signedIn: true } |
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.
we should add a snapshot when not signed in
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.
Done. tests should move into TmPage (which is barely tested) #2156
@@ -14,7 +14,8 @@ | |||
<slot slot="header-buttons" name="header-buttons" /> | |||
</tm-page-header> | |||
<main class="tm-page-main"> | |||
<template v-if="this.$slots['managed-body']"> | |||
<card-sign-in-required v-if="signinRequired && !session.signedIn" /> |
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.
should this go inside of the template below with connecting, loading, and error?
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.
either way. I can do it.
@@ -90,6 +93,10 @@ export default { | |||
refresh: { | |||
type: Function, | |||
default: undefined | |||
}, | |||
signInRequired: { |
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.
thinking ahead a little bit (maybe too far ahead) maybe this should be called addressRequired
because soon you will not need to "sign in" or "be signed in" - you will just have to provide an address.
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.
although the address will still be provided on the sign in session
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.
I could go either way
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.
let's merge it for now.
Some things were missing. Will add them to your PR |
Added a sign in required message
Readded the primary color to buttons.