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.

Sign up for GitHub

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

fixes #590: change isCompoundSelector to not match prop selector #595

Merged
merged 1 commit into from
Oct 12, 2016
Merged

fixes #590: change isCompoundSelector to not match prop selector #595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ export function selectorError(selector, type = '') {
);
}

export const isCompoundSelector = /([a-z]\.[a-z]|[a-z]\[.*\]|[a-z]#[a-z])/i;
export const isCompoundSelector = /^[\.#]?-?[_a-z]+[_a-z0-9-]*[\.\[#]/i;
Copy link
Member

@ljharb ljharb Sep 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

i wish we had commented regexps for situations like this. Let's see:

/
  ^               # start
  [\.#]?          # a dot or a hash, 0 or 1
  -?              # a hyphen, 0 or 1
  [_a-z]+         # any letter or underscore, 1 or more
  [_a-z0-9-]*     # any letter, underscore, number, or hyphen, 0 or more
  [\.\[#]         # any dot, left bracket, or hash
/i

Even after that I'm not really sure how to reason out what it's matching against. I wonder if we could write this as multiple different regexes, instead of trying to combine it into one case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even after that I'm not really sure how to reason out what it's matching against. I wonder if we could write this as multiple different regexes, instead of trying to combine it into one case?

Agreed, that's the approach I took with #591 because using a single regex was just too confusing. Breaking this into multiple regexes seems reasonable, but at a certain point it feels like we'd be implementing our own parser and I wonder if it would be better to just put the work into making #534.

Copy link
Member

Choose a reason for hiding this comment

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

That also seems reasonable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

[\.#]? # anything but a hash, 0 or 1
not sure it matters here but I think this one is actually '. or a #, 0 or 1'

Copy link
Member

Choose a reason for hiding this comment

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

Whoops, you're right. Will update my comment

Copy link

Choose a reason for hiding this comment

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

Actually you'll need a parser because CSS selectors are described using a LL(1) grammar and therefore cannot be parsed by a regular expression.

But you don't need to reinvent the wheel - use an existing selector engine like Sizzle for example and kill all the birds.. eh bugs with one stone.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could bring in xregexp if we wanted to make this regexp more readable with comments. I generally think that regexps should have comments, but I honestly don't find this one to be too bad.

Also, inside of brackets, I don't think you need to escape things like . and [, so you could remove a couple of backslashes to clean it up a bit.

Overall, this seems to be an improvement so I think we might as well merge it in. In the long run, if we want to support the full range of CSS selectors, I agree with @Jazen that we should bring in a parser.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lencioni we have an open PR for implementing a parser #458


const isPropSelector = /^\[.*\]$/;

Expand Down
44 changes: 44 additions & 0 deletions test/ShallowWrapper-spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,50 @@ describe('shallow', () => {
expect(wrapper.find('.foo').type()).to.equal('input');
});

it('should find an element that has dot in attribute', () => {
const wrapper = shallow(
<div>
<div data-baz="foo.bar" />
</div>
);

const elements = wrapper.find('[data-baz="foo.bar"]');
expect(elements.length).to.equal(1);
});

it('should find an element that with class and attribute', () => {
const wrapper = shallow(
<div>
<div data-baz="bar" className="classBar" />
</div>
);

const elements = wrapper.find('.classBar[data-baz="bar"]');
expect(elements.length).to.equal(1);
});

it('should find an element that with multiple classes and one attribute', () => {
const wrapper = shallow(
<div>
<div data-baz="bar" className="classBar classFoo" />
</div>
);

const elements = wrapper.find('.classBar.classFoo[data-baz="bar"]');
expect(elements.length).to.equal(1);
});

it('should find an element that with class and class with hyphen', () => {
const wrapper = shallow(
<div>
<div data-baz="bar" className="classBar class-Foo" />
</div>
);

const elements = wrapper.find('.classBar.class-Foo');
expect(elements.length).to.equal(1);
});

it('should find an element based on a tag name and class name', () => {
const wrapper = shallow(
<div>
Expand Down