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

@aexaey aexaey commented Feb 28, 2017

Copy link
Copy Markdown

No description provided.

@awlayton

awlayton commented Mar 7, 2017

Copy link
Copy Markdown
Collaborator

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

aexaey commented Mar 7, 2017

Copy link
Copy Markdown
Author

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

awlayton commented Mar 7, 2017

Copy link
Copy Markdown
Collaborator

I would have just made them .ifExists and .ifVisible

@aexaey

aexaey commented Mar 7, 2017

Copy link
Copy Markdown
Author

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

aexaey commented Mar 8, 2017

Copy link
Copy Markdown
Author

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.

@awlayton awlayton left a comment

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.

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