Skip to content

feat(ByRole): improve current option description #1273

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

Conversation

thefalked
Copy link

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:

  • Documentation added to the
    docs site N/A
  • Tests N/A
  • TypeScript definitions updated N/A
  • Ready to be merged

@codesandbox-ci
Copy link

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 5a45a3a:

Sandbox Source
react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Oct 13, 2023

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.

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

@MatanBobi
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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!

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