Skip to content

Add .if_exists()/.if_visible() (implements #266)#267

Open
aexaey wants to merge 3 commits into
johntitus:masterfrom
aexaey:master
Open

Add .if_exists()/.if_visible() (implements #266)#267
aexaey wants to merge 3 commits into
johntitus:masterfrom
aexaey:master

Conversation

@aexaey
Copy link
Copy Markdown

@aexaey aexaey commented Feb 28, 2017

No description provided.

@awlayton
Copy link
Copy Markdown
Collaborator

awlayton commented Mar 7, 2017

I'm not sure how I feel about these functions. They feel unnecessary to me.

If other people want them I might pull them in. Do you have an opinion @johntitus?

Also, the underscores in the name irk me, the do not match any of the other function names.

@aexaey
Copy link
Copy Markdown
Author

aexaey commented Mar 7, 2017

Regarding underscores - would this sort of syntax be more palatable?

.visible('#foo')
.if(function() {
        ...

i.e. add new if action that does the same as then, but conditionally.

@awlayton
Copy link
Copy Markdown
Collaborator

awlayton commented Mar 7, 2017

I would have just made them .ifExists and .ifVisible

@aexaey
Copy link
Copy Markdown
Author

aexaey commented Mar 7, 2017

Oh, I see. Cool. I've changed PR to use headlessCamelCase.

@johntitus
Copy link
Copy Markdown
Owner

Seems useful but I too hesitate to go too far down this sort of rabbit hole. I lean towards accepting this PR (and thanks for making it @aexaey) if tests are added.

@aexaey
Copy link
Copy Markdown
Author

aexaey commented Mar 8, 2017

Thanks @johntitus , I've added tests and bare-bones descriptions to Readme.md.
Let me know if you'd like this PR squashed into a single commit.

Copy link
Copy Markdown
Collaborator

@awlayton awlayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for contributing. I found some things.

Comment thread lib/actions.js
* @param {function} fn
*/
exports.ifExists = function(selector, fn) {
debug('.ifExists()', selector);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should just use the existing .exists() action rather than re-implementing it.

Comment thread lib/actions.js
* @param {function} fn
*/
exports.ifVisible = function(selector, fn) {
debug('.ifVisible()', selector);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, you should utilize the existing .visible() action.

Comment thread lib/actions.js
debug('.ifExists()', selector);
return this.count(selector).then(function(count) {
if (count > 0) {
return fn();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the indentation consistent, as I am neurotic about that.

Comment thread lib/actions.js
exports.ifExists = function(selector, fn) {
debug('.ifExists()', selector);
return this.count(selector).then(function(count) {
if (count > 0) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should pass the resolution value of the previous Promise to fn.

For example

...
    .return('foobar')
    .ifExists('selectorThatExists', function(res) {
        console.log(res); // Should log 'foobar'
    })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants