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

(DomActions) WTR tests for elementSelector() #495

Merged

Conversation

freiondrej
Copy link
Contributor

Hey @muodov, will it bring value to continue covering DomActions like this? I can try to send a PR every now and then :) or pls point me to some higher-value task 🙏

@muodov muodov self-requested a review October 15, 2024 08:27
Copy link
Member

@muodov muodov left a comment

Choose a reason for hiding this comment

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

This is great @freiondrej! I just had a couple minor remarks. Thank you for doing this!

tests-wtr/dom-actions/utils.ts Outdated Show resolved Hide resolved
])
})
})
describe('querySelectorChain', () => {
Copy link
Member

Choose a reason for hiding this comment

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

could you add a test for Shadow DOM? something for ['.my-shadow-root-selector', '.selector-inside-shadow-root']?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call! I'll adjust the should return elements by querySelectorAll from shadow root if available which now cannot be used directly anymore as the elementSelector() only accepts one argument.

@muodov
Copy link
Member

muodov commented Oct 21, 2024

@freiondrej do you want to follow up here?

@freiondrej
Copy link
Contributor Author

do you want to follow up here?

Yes, absolutely :) I just didn't have a free evening yet, apologies.

@freiondrej freiondrej requested a review from muodov October 21, 2024 18:06
@muodov muodov added patch Increment the patch version when merged tests Add or improve existing tests labels Oct 22, 2024
Copy link
Member

@muodov muodov left a comment

Choose a reason for hiding this comment

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

LGTM!

@muodov muodov merged commit 0a0054b into duckduckgo:main Oct 22, 2024
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged tests Add or improve existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants