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

feat: migrate suggestions/pretty-dom/helpers/role-helpers to ts #850

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

marcosvega91
Copy link
Member

@marcosvega91 marcosvega91 commented Dec 17, 2020

What:

Follow-up #848 and #614

I have migrated the following files:

  • suggestions.js
  • pretty-dom.js
  • helpers.js
  • role-helpers.js

Why:

How:

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • Typescript definitions updated
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 17, 2020

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 48cf918:

Sandbox Source
react-testing-library-examples Configuration

@marcosvega91
Copy link
Member Author

marcosvega91 commented Dec 17, 2020

The CI is in error ( I don't understand why it wasn't in the previous PR ). The Matcher type can be of type Regex, string or a function. In the test types a number is used, should we handle this case ?

EDIT:
Ok, I have found the issue related to the use of number #826

EDIT:
It was giving error because number was not used in the Matcher type and because were used types inside node_modules

@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #850 (3ea57ca) into master (5bc9364) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #850   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          951       970   +19     
  Branches       288       302   +14     
=========================================
+ Hits           951       970   +19     
Impacted Files Coverage Δ
src/helpers.ts 100.00% <100.00%> (ø)
src/label-helpers.ts 100.00% <100.00%> (ø)
src/pretty-dom.ts 100.00% <100.00%> (ø)
src/role-helpers.ts 100.00% <100.00%> (ø)
src/suggestions.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bc9364...48cf918. Read the comment docs.

src/__tests__/suggestions.ts Outdated Show resolved Hide resolved
src/__tests__/suggestions.ts Outdated Show resolved Hide resolved
src/helpers.ts Outdated Show resolved Hide resolved
src/label-helpers.ts Show resolved Hide resolved
@@ -238,7 +241,7 @@ it('should not suggest by label when using by label', async () => {
)

// if a suggestion is made, this call will throw, thus failing the test.
const password = await screen.findByLabelText(/bar/i)
const password = (await screen.findByLabelText(/bar/i)) as Element
Copy link
Member

Choose a reason for hiding this comment

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

question: Why do we need this type casting?

Copy link
Member Author

Choose a reason for hiding this comment

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

we don't have already converted findByLabelText so it will return an any type for now

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 do a

Suggested change
const password = (await screen.findByLabelText(/bar/i)) as Element
const password: Element = await screen.findByLabelText(/bar/i)

so that we're still safe once we type findByLabelText?

Copy link
Member Author

Choose a reason for hiding this comment

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

it will still remain a warning, maybe we can leave as is

src/helpers.ts Outdated Show resolved Hide resolved
src/suggestions.ts Outdated Show resolved Hide resolved
src/suggestions.ts Outdated Show resolved Hide resolved
@marcosvega91 marcosvega91 changed the title feat: migrate suggestions/pretty-dom/helpers to ts feat: migrate suggestions/pretty-dom/helpers/role-helpers to ts Dec 17, 2020
src/role-helpers.ts Outdated Show resolved Hide resolved
src/role-helpers.ts Outdated Show resolved Hide resolved
@nickmccurdy
Copy link
Member

@marcosvega91 If it's not too much work would you mind extracting the CI fix into a separate PR so we can merge that first?

@marcosvega91
Copy link
Member Author

Yes, sure

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.

None yet

4 participants