testing-library / dom-testing-library

πŸ™ Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.28k stars 467 forks source link

feat(ByRole): improve `current` option description #1273

Closed thefalked closed 1 year ago

thefalked commented 1 year ago

What:

Updating current options JSDOC to be more clear how to use it.

Why:

Isn't clear reading the docs that we could use another value, even with the string type.

How:

Changing the JSDOC description on types/queries.d.ts file.

Checklist:

codesandbox-ci[bot] commented 1 year ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5a45a3aecf9bdb2676ecc204b0a8594e519a4167:

Sandbox Source
react-testing-library-examples Configuration
codecov[bot] commented 1 year ago

Codecov Report

Merging #1273 (5a45a3a) into main (a7b7252) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##              main     #1273   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         1038      1038           
  Branches       346       346           
=========================================
  Hits          1038      1038           
Flag Coverage Ξ”
node-14 100.00% <ΓΈ> (ΓΈ)
node-16 100.00% <ΓΈ> (ΓΈ)
node-18 100.00% <ΓΈ> (ΓΈ)
node-20 100.00% <ΓΈ> (ΓΈ)

Flags with carried forward coverage won't be shown. Click here to find out more.

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

MatanBobi commented 1 year ago

Hi @thefalked, thanks for taking the time to create this one. I don't think that this is better than what we have (based on the type (boolean | string) and the current comment.

I'm leaning towards keeping it as it is but I'll leave it open if any other maintainer has a different opinion. Thanks again.

thefalked commented 1 year ago

Hi @thefalked, thanks for taking the time to create this one. I don't think that this is better than what we have (based on the type (boolean | string) and the current comment.

I'm leaning towards keeping it as it is but I'll leave it open if any other maintainer has a different opinion. Thanks again.

The only reason that i though it was a good idea to change is that in the docs site and in the JSDOC description is only talking about the true & false, i spend my hole day trying to find why the test was failing and i had aria-current="page". (I though that current: true would validate if the property was on the element independent of the value).

Maybe someone who has less experience like me find this a better solution than what was on the previous description.

MatanBobi commented 1 year ago

If that's the case, I think it will be better to update our docs and not the JSDOC comment. Any chance you can open a PR to our docs site for this one? I'm closing this as I highly believe that this should be a part of the doc and not the JSDOC.

Thanks!