-
Notifications
You must be signed in to change notification settings - Fork 98
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
Jordan/388 profile #449
Jordan/388 profile #449
Conversation
…idator and candidate profiles,
Codecov Report
@@ Coverage Diff @@
## develop #449 +/- ##
=========================================
+ Coverage 75.79% 76.2% +0.4%
=========================================
Files 91 90 -1
Lines 1409 1387 -22
Branches 69 66 -3
=========================================
- Hits 1068 1057 -11
+ Misses 321 312 -9
+ Partials 20 18 -2
|
Ah this may have been better made on top of my #427 PR. A lot of the changes are duplicated. |
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.
Could you add screenshots of the changed pages plz?
computed: { | ||
...mapGetters(['user']), | ||
userInfo () { | ||
console.log(this.user) |
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.
was it intended to leave this in? userInfo will be null I guess and is not used in the component
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.
no. i will remove.
div(slot="menu"): tool-bar | ||
router-link(to="/validators" exact) | ||
i.material-icons arrow_back | ||
.label Back | ||
|
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.
so this will be a completely empty page? is this one of the components we don't use anymore?
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 will delete this file.
let wrapper, store, router | ||
let {mount} = setup() | ||
let wrapper | ||
let store = new Vuex.Store({ |
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.
why did you choose to remove the helper?
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.
assuming i understand correctly... because the helper installs localVue.use(VueRouter)
. $route
becomes read only and cannot be set manually (overridden) in the test.
https://vue-test-utils.vuejs.org/en/guides/using-with-vue-router.html
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.
but before it worked with router.push('/staking/delegates/pubkeyX')
or not?
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.
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 think it worked before with router.push
because there were no params involved.
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.
The route is defined as /staking/delegates/:delegate
:delegate
is the param you want to get in the component. When you push the route /staking/delegates/pubkeyX
it will set the param delegate
to pubkeyX
. This is how it worked before.
As already mentioned, I would like to change the test setup back to using vuex-setup for consistency and to also test vuex-actions existence (lite e2e testing).
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.
OH! ok! let me try that out. that would be better!
this is a PR that simplifies the user profile page and the delegate profile page. both candidates and validators can now use
PageDelegate
as their profile pages. i also removed a lot of unused code.closes #388