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

Improve engine view matcher with new pattern syntax #9694

Merged
merged 28 commits into from
Jun 14, 2021
Merged

Conversation

maxbarnas
Copy link
Contributor

@maxbarnas maxbarnas commented May 13, 2021

Suggested merge commit message (convention)

Feature (engine): Improved engine view matcher with new pattern syntax allowing to match attribute keys using regular expressions. Unified pattern syntax between attributes, styles, and classes. Closes #9872.

Feature (engine): Added special expand option to StylesMap.getStyleNames() and view Element.getStyleNames() methods allowing to expand shorthand style properties.


Additional information

@maxbarnas maxbarnas requested a review from jacekbogdanski May 13, 2021 13:30
@maxbarnas maxbarnas force-pushed the internal/718 branch 3 times, most recently from 82fedd6 to b061792 Compare May 13, 2021 20:08
Copy link
Member

@jacekbogdanski jacekbogdanski left a comment

Choose a reason for hiding this comment

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

Looks pretty good 👍🏻

TBC

packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
Comment on lines 365 to 371
// eslint-disable-next-line dot-notation
if ( isPlainObject( pattern ) && pattern[ 'key' ] && pattern[ 'value' ] ) {
return pattern;
}

// Assume the pattern is either String or RegExp.
return { key: pattern, value: true };
Copy link
Member

Choose a reason for hiding this comment

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

What if someone provided pattern = { value: 'red' } or pattern = { key: 'color' } ? I'm wondering if we need some handling for that, maybe throw an error if pattern is an object but doesn't include key and value properties?

I'm not sure if checking pattern[property] is enough, as it may have falsy value like pattern = { key: 'data-foo', value: '' }

Copy link
Member

Choose a reason for hiding this comment

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

It would be also good to have some test coverage for that.

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 to not be resolved yet.

packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
@jacekbogdanski jacekbogdanski self-requested a review June 2, 2021 09:23
packages/ckeditor5-engine/src/view/element.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
Comment on lines 365 to 371
// eslint-disable-next-line dot-notation
if ( isPlainObject( pattern ) && pattern[ 'key' ] && pattern[ 'value' ] ) {
return pattern;
}

// Assume the pattern is either String or RegExp.
return { key: pattern, value: true };
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 to not be resolved yet.

packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/stylesmap.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/view/matcher.js Outdated Show resolved Hide resolved
Copy link
Member

@jacekbogdanski jacekbogdanski left a comment

Choose a reason for hiding this comment

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

Seems like we are also missing unit tests for regexp/string pattern full properties, like:

const match = {
  attributes: /^data-.*$/,
  styles: 'margin'
}

@jacekbogdanski jacekbogdanski self-requested a review June 8, 2021 13:32
@maxbarnas maxbarnas marked this pull request as ready for review June 8, 2021 13:33
packages/ckeditor5-engine/src/view/element.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/matcher.js Show resolved Hide resolved
packages/ckeditor5-engine/src/view/stylesmap.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/view/stylesmap.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/view/matcher.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/view/matcher.js Outdated Show resolved Hide resolved
@jacekbogdanski
Copy link
Member

jacekbogdanski commented Jun 10, 2021

@maxbarnas I've added a test in 526d5d7 verifying if matcher returns the unique result for similar patterns. It wasn't the case previously as it was not possible to match the attribute key multiple times, but now this test fails.

Uh, actually I see that a similar test for class names (on master) will duplicate results, so it seems like an expected case and we shouldn't introduce breaking changes in this PR. I will correct the previous commit.

@jacekbogdanski
Copy link
Member

@maxbarnas I've rewritten a bit of documentation for new matcher options. It's more organized now so should be more readable for users. Could you take a look at these changes if everything is ok? 84f971f

Copy link
Member

@jacekbogdanski jacekbogdanski left a comment

Choose a reason for hiding this comment

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

🎉

@jacekbogdanski jacekbogdanski merged commit 22fad3d into master Jun 14, 2021
@jacekbogdanski jacekbogdanski deleted the internal/718 branch June 14, 2021 13:53
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.

Improve Matcher to allow matching attribute keys by regexp and unify API
3 participants