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

refactor: break up context.js and clean it up a bit #3794

Merged
merged 2 commits into from
Nov 23, 2022

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Nov 22, 2022

In refactoring the context file I found this bug:

Related to #3793

@WilcoFiers WilcoFiers requested a review from a team as a code owner November 22, 2022 13:00
@WilcoFiers WilcoFiers marked this pull request as draft November 22, 2022 13:07
@WilcoFiers WilcoFiers changed the title fix(context): Allow multiple strings for include/exclude refactor: break up context.js and clean it up a bit Nov 23, 2022
Comment on lines -185 to -191
context.frames.forEach((frame, i) => {
if (frame.include.length === 0) {
return new Error(
'No elements found for include in Context of frame ' + i
);
}
});
Copy link
Contributor Author

@WilcoFiers WilcoFiers Nov 23, 2022

Choose a reason for hiding this comment

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

I left this out because it never worked. We're returning in a forEach. That does nothing. This is completely pointless too. To test the entire frame, include has to be empty. An empty include is a valid option.

@WilcoFiers WilcoFiers marked this pull request as ready for review November 23, 2022 13:58
@WilcoFiers WilcoFiers merged commit 757a6fc into develop Nov 23, 2022
@WilcoFiers WilcoFiers deleted the context-refactor branch November 23, 2022 21:06
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.

2 participants