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 specifying a predicate to find a specific element #38

Merged
merged 9 commits into from
Oct 5, 2021

Conversation

bengry
Copy link
Contributor

@bengry bengry commented Oct 3, 2021

fixes #37

index.js Outdated Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
@sindresorhus
Copy link
Owner

This will need readme updates and tests.

@sindresorhus
Copy link
Owner

It should also be added to observeReadyElements.

@bengry
Copy link
Contributor Author

bengry commented Oct 4, 2021

This will need readme updates and tests.

I added this to the readme, but I can't run the tests locally since it seems like the typescript package is missing from devDependencies. Is that on purpose? I'm not sure how they pass in CI. I added some tests (see 77b3176), but couldn't run them.

It should also be added to observeReadyElements.

Done

@bengry
Copy link
Contributor Author

bengry commented Oct 4, 2021

On an unrelated note, is there a reason to maintain a separate .d.ts file, instead of re-writing the package in TypeScript? It's not large and a re-write should be simple enough.

@sindresorhus
Copy link
Owner

On an unrelated note, is there a reason to maintain a separate .d.ts file, instead of re-writing the package in TypeScript?

Yes. I don't like TS. I only provide types for user convenience. (sindresorhus/ky#321 (comment))

readme.md Outdated Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Generally, don't force push in pull requests, it also makes it harder to review what changed.

@bengry
Copy link
Contributor Author

bengry commented Oct 4, 2021

You overwrote my changes: 8dc7b0c24363479246a6a2b4b3cf3d6da6f968c4..ed957e569374df606507811648b6f2730b983a2c (compare)

I've re-added them in add7485. If I understood correctly - with the last push all that was overridden was the missing import elementReady from 'element-ready'; from the docs. Since there were both this and the suggestions (which I've implemented separately) in the diffs.

Please LMK if I missed anything.

@sindresorhus
Copy link
Owner

There were a few other changes too. Just look at the diff I commented.

@bengry
Copy link
Contributor Author

bengry commented Oct 4, 2021

There were a few other changes too. Just look at the diff I commented.

Right, but there were also the suggestion, so I think that together we're all aligned now: https://github.com/sindresorhus/element-ready/compare/8dc7b0c24363479246a6a2b4b3cf3d6da6f968c4..add7485

@sindresorhus
Copy link
Owner

I committed some small tweaks.

@sindresorhus
Copy link
Owner

The "check elements against a predicate" test is failing though.

@bengry
Copy link
Contributor Author

bengry commented Oct 4, 2021

The "check elements against a predicate" test is failing though.

Fixed in cd1f75a. There were some errors due to some renaming I did while working on the tests, and using a non-existent property on the element in the assertion. Funnily enough, these errors would've been caught by TypeScript.

@sindresorhus sindresorhus merged commit 78127f9 into sindresorhus:main Oct 5, 2021
@sindresorhus
Copy link
Owner

Thanks :)

@bengry bengry deleted the feature/element-predicate branch October 5, 2021 05:42
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.

Allow specifying a predicate function to only resolve on a more specific element
2 participants