Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Fix wp-class and introduce the evaluate function #127

Merged
merged 12 commits into from
Jan 10, 2023

Conversation

luisherranz
Copy link
Member

What

I improved the wp-class implementation because it was very basic. My idea was also to add unit tests, but instead I added more e2e tests. I also took the opportunity to improve the logic that resolves the path.

Why

Because the implementation was so basic that it could fail with very simple cases.

How

For wp-class I have introduced regular expressions. For the e2e tests I followed what I did in my previous pull request. And for the logic that resolves path, I have introduced a new function evaluate.

I did a video to explain in detail each change:

https://www.loom.com/share/2fd4cbd8963f416da49b71c484d68b65

Copy link
Collaborator

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

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

Looks good to me, @luisherranz! 👌

PS: I would have added some e2e tests for class names contained inside others (e.g., "foo foo-bar") or just part of a class name (e.g. foo inside "foo-bar foo-baz") to ensure that the regexp only handle whole class names. This is something you mentioned in the video, so I was expecting these kinds of tests to be included. 🙂

@luisherranz
Copy link
Member Author

I would have added some e2e tests for class names contained inside others (e.g., "foo foo-bar")

I'll add them before merging. Thanks, David!

Copy link
Collaborator

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

Looks 💯 ! TIL about the Playwright VSCode extension!

@luisherranz
Copy link
Member Author

@DAreRodz: is 464b4eb enough? I don't fully understand what you mean by foo inside "foo-bar foo-baz".

@DAreRodz
Copy link
Collaborator

@DAreRodz: is 464b4eb enough? I don't fully understand what you mean by foo inside "foo-bar foo-baz".

Thank @luisherranz! I think it's enough.

That test would have been just to ensure that the directive doesn't mess with other classes containing the one defined for the directive, even when that class is not there yet. But I guess that is already covered with the first test. 😊

@luisherranz luisherranz merged commit 8382341 into main-wp-directives-plugin Jan 10, 2023
@luisherranz luisherranz deleted the wp-class-fix-and-e2e-tests branch January 10, 2023 17:03
@luisherranz
Copy link
Member Author

Merged then 🙂

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants