-
Notifications
You must be signed in to change notification settings - Fork 125
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 isVisible() and isNotVisible() assertions #54
Conversation
@@ -16,5 +16,12 @@ test('qunit-dom assertions are available', function(assert) { | |||
assert.dom('#title').exists(); | |||
assert.dom('#subtitle').doesNotExist(); | |||
assert.dom('#title').hasText('Welcome to Ember'); | |||
|
|||
// isVisible/isNotVisible tests | |||
assert.dom('#title').isVisible(); |
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 needed to add the isVisible/isNotVisible tests here as the jest tests always seems to consider elements as hidden. Although I may have been doing something incorrect?
dddcd01
to
42e719f
Compare
42e719f
to
1545089
Compare
Just fixed some failing tests, they are now ✅ |
// https://github.com/jquery/jquery/blob/4a2bcc27f9c3ee24b3effac0fbe1285d1ee23cc5/src/css/hiddenVisibleSelectors.js#L11-L13 | ||
|
||
export default function visible(el) { | ||
return !!(el.offsetWidth || el.offsetHeight || el.getClientRects().length); |
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 make this check a little more explicit? assuming I understand the logic correctly, maybe something like this:
return el.offsetWidth !== 0 || el.offsetHeight !== 0 || el.getClientRects().length !== 0
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 seems like a good change, though the above is literally what jQuery uses (though that isn't any endorsement for using that code).
@@ -462,6 +463,58 @@ export default class DOMAssertions { | |||
this.hasNoValue(message); | |||
} | |||
|
|||
/** | |||
* Assert an [HTMLElement][] matching the `selector` is visible. |
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.
can you add to the documentation what makes an element "visible"? for example if the browser is scrolled to a different part of the page does that mean that elements are not visible then?
I tried to push some of these fixes to @patocallaghan's fork, but no dice. Guess we'll have to wait. |
So one thing that needs to be addressed here is that this code will fail if an element is not rendered yet, which may still be considered not visible. In jQuery, you always get an element back. So, the visible check works well because if jQuery doesn't find the element, it returns a default jQuery object which can be evaluated. With using To fix return el !== null && (el.offsetWidth !== 0 || el.offsetHeight !== 0 || el.getClientRects().length !== 0); To fix return el === null || !(el.offsetWidth !== 0 || el.offsetHeight !== 0 || el.getClientRects().length !== 0); |
Apologies I hope to get to addressing the comments and finishing this in the next few days |
Let me know what i can do to help. |
Closing this PR as #67 supersedes it |
Fixes #52
Adds
isVisible
andisNotVisible
assertion helpers.Visibility is modelled off of jQuery's
:visible
logic in jQuery 3.0 https://github.com/jquery/jquery/blob/4a2bcc27f9c3ee24b3effac0fbe1285d1ee23cc5/src/css/hiddenVisibleSelectors.js#L11-L13From https://api.jquery.com/hidden-selector/, an element is considered "hidden" if it follows any of the following criteria
As mentioned in the issue you may decide not to merge this. If you do, I can then update the README information.