WIP: Use page objects for user-sidebar-test #912
Conversation
sarupbanskota
force-pushed
the
tests/page-object-user-sidebar
branch
from
6b37055 to
b73275a
Compare
| import { count, text, isHidden } from 'ember-cli-page-object'; | ||
|
|
||
| export default { | ||
| scope : '.user-sidebar', |
There was a problem hiding this comment.
could it be because count('.user-sidebar') will look for .user-sidebar within the scope (which is .user-sidebar)?
sarupbanskota
added
needs work
help wanted
in progress
and removed
help wanted
needs work
needs code review
labels
sarupbanskota
force-pushed
the
tests/page-object-user-sidebar
branch
from
a2783de to
1f6e413
Compare
begedin
left a comment
begedin
left a comment
There was a problem hiding this comment.
Looks good. Only a couple of minor changes and then we're good to go.
| assert.equal(this.$('li.twitter a').attr('href'), 'https://twitter.com/joshsmith', 'The twitter link renders'); | ||
| assert.equal(this.$('li.website').text().trim(), 'https://codecorps.org', 'Their website renders'); | ||
| assert.equal(this.$('li.website a').attr('href'), 'https://codecorps.org', 'The website link renders'); | ||
| assert.equal(page.userSidebarCount, 1, 'Component\'s element is rendered'); |
There was a problem hiding this comment.
We don't really need this assertion at all.
| export default { | ||
| scope : '.user-sidebar', | ||
|
|
||
| userSidebarCount : isVisible(), |
There was a problem hiding this comment.
I don't think we need this property. I would name it userSidebarVisible or userSidebarRendered in context, but if the removal of the "it renders" test above and the one assertion in the other test means we no longer use this property, just remove it from here.
| twitterHandleHidden : isHidden('li.twitter'), | ||
| websiteHidden : isHidden('li.website'), | ||
| twitterLink : property('href', 'li.twitter a'), | ||
| websiteLink : property('href', 'li.website a') |
There was a problem hiding this comment.
There are arguments for this type of formatting/spacing, but we don't use it anywhere in the app ourselves, so we would prefer if you switched this to just using a single space instead of horizontally aligning the values.
There was a problem hiding this comment.
alright cool. I was actually supposed to introduce that with #653 which got stale and I'll do another PR, but I thought I should do it once the remaining of my PRs are merged.
|
@begedin thanks for the feedback! NY time so a bit slow, but I should clear this up in a day or two. |
joshsmith
force-pushed
the
tests/page-object-user-sidebar
branch
from
1f6e413 to
9d10c59
Compare
joshsmith
force-pushed
the
tests/page-object-user-sidebar
branch
from
9d10c59 to
e975e9c
Compare
Closes #776
Also under #697