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

Add support for multiple matching blocks #243

Merged
merged 4 commits into from
Nov 9, 2022
Merged

Conversation

codr
Copy link
Contributor

@codr codr commented Nov 4, 2022

No description provided.

@mirisuzanne
Copy link
Member

Hi @codr thanks for contributing! Can you provide some background on the use-case here, since it's not a feature that's been discussed? It would also be great to have updates to the changelog and documentation, so that people know about this new functionality, and how to use it.

@codr
Copy link
Contributor Author

codr commented Nov 4, 2022

Hello, Thanks for the replay.

The use-case is: when testing Sass with multiple block with matching selectors, only the first block can be accounted for.
I tried to capture this in the unit test. Let me know if I can expand on that. should I be concerned about other edge cases?

update test to make sure it checks the second block
Add support for `contians` when multiple matching selectors exists
- Update changelog message
- ran `yarn commit`
@mirisuzanne
Copy link
Member

The use-case is: when testing Sass with multiple block with matching selectors, only the first block can be accounted for. I tried to capture this in the unit test. Let me know if I can expand on that.

Makes sense to me!

should I be concerned about other edge cases?

The only potential edge-case I can think of (but may not be an issue) is if it's possible to have accidental bleed across multiple tests that use the same selector? I'm not as familiar with the JS parsing step here, so maybe @jgerigmeyer can speak to that. Otherwise, it looks good to me. Thanks again!

(We may want to document this in either the readme or the contains mixin. If you want to do that, it would be great - but I don't think it needs to hold up this PR. We can handle it sometime before the next release.)

@codr
Copy link
Contributor Author

codr commented Nov 9, 2022

We may want to document this in either the readme or the contains mixin.

I'm happy to add some documentation. I'm not entirely sure where or what. Looking at this mixing comment, I think this was the original intent. Maybe this is more of a bug fix than a feature?

@mirisuzanne
Copy link
Member

Maybe this is more of a bug fix than a feature?

Oh yeah, that seems fair. Maybe we can leave the documentation as-is.

@jgerigmeyer
Copy link
Member

@codr Thanks! 🚀

@jgerigmeyer jgerigmeyer merged commit 3ecec59 into oddbird:main Nov 9, 2022
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