Skip to content
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

Contextual css selector #217

Merged
merged 1 commit into from
May 7, 2016
Merged

Conversation

dustinsanders
Copy link
Contributor

Issue #23.

Added support for descendent, >, ~, and +.

};
};

let { predicate, selectSiblings } = {};
Copy link
Member

Choose a reason for hiding this comment

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

this seems like a pretty sketchy way to avoid let predicate; let selectSiblings;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually do this when defining multiple variables to undefined, but let predicate; let selectSiblings; is also fine

buildInstPredicate,
findWhereUnwrapped,
childrenOfInst
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a good argument for this to be a class instead of just pure functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I was currying it but it didn't seem as clean.

complexSelector(buildInstPredicate, findWhereUnwrapped, childrenOfInst)(selector, this);

I guess it could all be one function, but I wanted to separate initialization from the logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ultimately it's up to the maintainers. They might see this argument as moot. But it could make maintenance easier when all selector engines follow a similar flow. Previously that was the case until here.

Additionally, it is heavier to instantiate an object for every selector. But I don't they worry too much about the perf for this lib.

@blainekasten
Copy link
Contributor

Does this only support 1 level of nested selectors? Will it be extra hard to add infinite depth to nested selectors later on if/when wanted?

e.g.,

find('.foo + div > span')

@dustinsanders
Copy link
Contributor Author

@blainekasten I just ran the following

it.only('.foo + div > span', () => {
    const wrapper = shallow(
      <div>
        <div className="foo" />
        <div>
          <span />
        </div>
      </div>
    );

    expect(wrapper.find('.foo + div > span').length).to.equal(1);
  });

and it passed. Based on how it is written it should support nesting, but there should be more testing around the nested selectors.

@dustinsanders
Copy link
Contributor Author

See changes made based off of suggestions

@@ -0,0 +1,126 @@
import reduce from 'lodash/reduce';

Choose a reason for hiding this comment

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

Array.prototype.reduce is available in ES5

@dustinsanders
Copy link
Contributor Author

@borisyankov good call, using native reduce in latest commit

@koba04
Copy link
Contributor

koba04 commented Apr 8, 2016

any progress on this?

@dustinsanders
Copy link
Contributor Author

I think I've addressed all the concerns, but am open to other changes

@ljharb
Copy link
Member

ljharb commented Apr 8, 2016

@dustinsanders would you mind rebasing this on top of latest master, and down to a smaller number of conceptually atomic commits? It's got merge conflicts atm.

@dustinsanders
Copy link
Contributor Author

@ljharb I have rebased and squashed to one commit

@ljharb
Copy link
Member

ljharb commented Apr 30, 2016

@dustinsanders looks like this needs a fresh rebase - sorry this has taken so long.

@dustinsanders
Copy link
Contributor Author

@ljharb this has been freshly rebased

@lelandrichardson
Copy link
Collaborator

lelandrichardson commented May 7, 2016

@dustinsanders this looks awesome. great work. (Sorry this took so long for me to get to it)

@lelandrichardson lelandrichardson merged commit 77eb751 into enzymejs:master May 7, 2016
mathieuancelin pushed a commit to mathieuancelin/enzyme that referenced this pull request May 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants