feat: ...byRole type queries accepts second arg #875

kiranjd · 2021-12-02T14:51:56Z

Closes #827

accepts a 'name' property to refine query

Summary

Test plan

src/helpers/a11yAPI.js

src/helpers/makeA11yQuery.js

src/__tests__/a11yAPI.test.js

thymikee

Looks like it's a good direction. Please take a look at 31ef0b9 to see how to resolve API changes in a backward compatible way

…react-native-testing-library into feat/by-role-second-arg

package.json

AugustinLF · 2022-03-28T15:17:16Z

@kiranjd is it something you're still interested in getting in? This is a feature that is clearly lacking for our current API, and I'd love to help you getting that in a mergeable state.

kiranjd · 2022-03-28T15:35:57Z

@AugustinLF Missed following up on this. I remember I had a few questions. I'll post the questions and follow it up to finish within this week. Thank you 🙏

…react-native-testing-library into feat/by-role-second-arg

kiranjd · 2022-03-30T10:55:07Z

src/helpers/makeA11yQuery.js

 import type { WaitForOptions } from '../waitFor';
 import {
  ErrorWithStack,
  prepareErrorMessage,
  createQueryByError,
 } from './errors';

+type QueryOptions = {


The name in QueryOptions type for getBy is optional. But, it's a must for the function, filterWithName. Is this duplication that I get rid of or is this okay?
(I haven't worked with types all that much 😬 )

- adds tests for deprecations - change second param on findBy queries

…ing-library into feat/by-role-second-arg

package.json

src/helpers/a11yAPI.ts

AugustinLF · 2022-03-31T15:44:22Z

src/helpers/makeA11yQuery.ts

+  'interval',
+  'stackTraceError',
+];
+const warnDeprectedWaitForOptionsUsage = (queryOptions?: WaitForOptions) => {


Suggested change

const warnDeprectedWaitForOptionsUsage = (queryOptions?: WaitForOptions) => {

const warnDeprecatedWaitForOptionsUsage = (queryOptions?: WaitForOptions) => {

Also pls move it to helpers/errors.ts, as this looks like reusable function that can be used with other findByXxx queries if they decide to go with query options.

src/__tests__/a11yAPI.test.tsx

AugustinLF · 2022-04-25T09:14:50Z

@kiranjd Do you need any help, is there anything I can do to help you getting that in? :)

mdjastrzebski · 2022-04-28T21:16:55Z

src/helpers/a11yAPI.ts

@@ -19,6 +47,10 @@ type QueryAllReturn = Array<ReactTestInstance>;
 type FindReturn = Promise<GetReturn>;
 type FindAllReturn = Promise<GetAllReturn>;

+export type QueryOptions = {


This type should be renamed to ByRoleOptions bacause this type of option is only relevant to byRole queries.

mdjastrzebski · 2022-04-28T21:20:28Z

src/helpers/makeA11yQuery.ts

+      isNodeValid(node) && matcherFn(node.props[name], matcher);
+
+    return (
+      matchesRole && !!getQueriesForElement(node).queryByText(options.name)


it looks like we should match for either queryByText(options.name) or queryByLabelText(options.name)

mdjastrzebski · 2022-04-28T21:23:08Z

src/helpers/makeA11yQuery.ts

@@ -31,8 +65,27 @@ const makeA11yQuery = <P extends unknown, M extends unknown>(
  queryNames: QueryNames,
  matcherFn: (prop: P, value: M) => boolean
 ) => (instance: ReactTestInstance) => {
-  const getBy = (matcher: M) => {
+  const filterWithName = (


Suggested name filterByAccessibilityName

mdjastrzebski · 2022-05-04T10:28:14Z

Let's pause this PR for couple of days till we get a11y query reorganisation from #325 done. This should allow for easy separate query options type per each query, so that the name options will be allowed only for byRole queries and not all a11y queries.

kiranjd · 2022-05-05T17:49:32Z

Yes, those changes caused lot of conflicts

On Wed, 4 May 2022 at 3:58 PM, Maciej Jastrzebski ***@***.***> wrote: Let's pause this PR for couple of days till we get a11y query reorganisation from #325 <#325> done. This should allow for easy separate query options type per each query, so that the name options will be allowed only for byRole queries and not all a11y queries. — Reply to this email directly, view it on GitHub <#875 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGFANA3PQMXZBXCJ744WUCDVIJGMTANCNFSM5JHKFNKQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Kiran JD

kiranjd · 2022-07-25T13:34:20Z

@mdjastrzebski is this a good time to take this PR further? If so, should I raise a separate PR to continue the work?

AugustinLF · 2022-07-28T09:27:24Z

@kiranjd I feel like yes we should be able to start working on that once again. I'd however wait for the go from @mdjastrzebski who might have something else planned before that. Nonetheless, feel free to let me know if you need any help on that, I'm very keen in getting this one in.

mdjastrzebski · 2022-08-01T10:37:43Z

@kiranjd go ahead 👍🏻 Now is the right time to work on this PR, as we've got a11y properly refactored.

AugustinLF · 2022-08-25T13:02:43Z

@kiranjd is there anything we can do to help you to get that in? It'd bring so much value, I'm really looking to get that one in <3

kiranjd · 2022-08-25T19:32:16Z

@AugustinLF Thanks for the support. I will work on this and hopefully get this in a mergeable state by this weekend 🤞🏻

AugustinLF · 2022-09-14T11:03:47Z

@kiranjd if you're short on time to finish this PR I can definitely help fixing the last few things to get it merged, let me know if you want help :)

kiranjd · 2022-09-15T16:36:07Z

@AugustinLF Have tried to redo the changes from the last state when it was working. There seems to be a lot of refactoring undergone in the meanwhile.
I would really appreciate your help in taking this further. Thank you for understanding :)

mdjastrzebski · 2022-09-16T12:43:08Z

@kiranjd yeah RNTL codebase did move a lot since the original PR submission :-)

@AugustinLF I would be great if you could help to move this forward, as byRole query with name option has a huge potential to be one of the top recommended queries as in the case of RTL: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#not-using-byrole-most-of-the-time

mdjastrzebski · 2022-09-22T12:36:48Z

This PR has been rebased and updated by @AugustinLF as #1127. Thank you @kiranjd 👏🏻 for the initial implementation!

kiranjd mentioned this pull request Dec 2, 2021

getByRole should accept a second argument to refine query as in react-testing-library #827

Closed

fix: remove unwanted files

ea68aa1

kiranjd force-pushed the feat/by-role-second-arg branch from ca7b4c7 to ea68aa1 Compare December 2, 2021 15:01

thymikee reviewed Dec 6, 2021

View reviewed changes

src/helpers/a11yAPI.js Outdated Show resolved Hide resolved

thymikee reviewed Dec 6, 2021

View reviewed changes

src/helpers/makeA11yQuery.js Outdated Show resolved Hide resolved

thymikee reviewed Dec 6, 2021

View reviewed changes

src/__tests__/a11yAPI.test.js Outdated Show resolved Hide resolved

thymikee reviewed Dec 6, 2021

View reviewed changes

src/__tests__/a11yAPI.test.js Outdated Show resolved Hide resolved

thymikee requested changes Dec 6, 2021

View reviewed changes

kiranjd added 8 commits December 7, 2021 01:14

deduplicate code for matching name

28a342c

adds tests for ...AllbyRole second arg

0713c95

update param to accept arity of 3 with waitFor as last arg

1360487

fix: remove unwanted files

f1e2c68

deduplicate code for matching name

30644cb

adds tests for ...AllbyRole second arg

cf0726f

update param to accept arity of 3 with waitFor as last arg

9c490c7

Merge branch 'feat/by-role-second-arg' of https://github.com/kiranjd/…

ae3d062

…react-native-testing-library into feat/by-role-second-arg

thymikee reviewed Jan 10, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

Update package.json

48c4e3b

AugustinLF added the compat: testing-library label Mar 28, 2022

kiranjd added 2 commits March 30, 2022 16:19

adds compulsory type for filterWithName function

949d559

Merge branch 'feat/by-role-second-arg' of https://github.com/kiranjd/…

0405619

…react-native-testing-library into feat/by-role-second-arg

kiranjd commented Mar 30, 2022

View reviewed changes

support backward compatibility for findBy*

29c2d6f

- adds tests for deprecations - change second param on findBy queries

kiranjd marked this pull request as ready for review March 30, 2022 11:54

kiranjd added 2 commits March 30, 2022 19:02

Merge branch 'main' of https://github.com/callstack/react-native-test…

1f725c7

…ing-library into feat/by-role-second-arg

migrate flow types to ts

a5f168c

kiranjd force-pushed the feat/by-role-second-arg branch from 291ebcc to a5f168c Compare March 30, 2022 13:35

making ts compiler happy

9ff3136

undo changes to other APIs

b6abc2f

AugustinLF requested changes Apr 4, 2022

View reviewed changes

mdjastrzebski reviewed Apr 28, 2022

View reviewed changes

AugustinLF mentioned this pull request Sep 18, 2022

Add support for ByRole with name #1127

Merged

mdjastrzebski closed this Sep 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ...byRole type queries accepts second arg #875

feat: ...byRole type queries accepts second arg #875

kiranjd commented Dec 2, 2021 •

edited by thymikee

Loading

thymikee left a comment

AugustinLF commented Mar 28, 2022

kiranjd commented Mar 28, 2022

kiranjd Mar 30, 2022 •

edited

Loading

AugustinLF Mar 31, 2022

mdjastrzebski Apr 28, 2022

AugustinLF commented Apr 25, 2022

mdjastrzebski Apr 28, 2022

mdjastrzebski Apr 28, 2022

mdjastrzebski Apr 28, 2022

mdjastrzebski commented May 4, 2022

kiranjd commented May 5, 2022 via email

kiranjd commented Jul 25, 2022

AugustinLF commented Jul 28, 2022

mdjastrzebski commented Aug 1, 2022

AugustinLF commented Aug 25, 2022 •

edited

Loading

kiranjd commented Aug 25, 2022

AugustinLF commented Sep 14, 2022

kiranjd commented Sep 15, 2022

mdjastrzebski commented Sep 16, 2022

mdjastrzebski commented Sep 22, 2022

	const warnDeprectedWaitForOptionsUsage = (queryOptions?: WaitForOptions) => {
	const warnDeprecatedWaitForOptionsUsage = (queryOptions?: WaitForOptions) => {

feat: ...byRole type queries accepts second arg #875

feat: ...byRole type queries accepts second arg #875

Conversation

kiranjd commented Dec 2, 2021 • edited by thymikee Loading

Summary

Test plan

thymikee left a comment

Choose a reason for hiding this comment

AugustinLF commented Mar 28, 2022

kiranjd commented Mar 28, 2022

kiranjd Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

AugustinLF Mar 31, 2022

Choose a reason for hiding this comment

mdjastrzebski Apr 28, 2022

Choose a reason for hiding this comment

AugustinLF commented Apr 25, 2022

mdjastrzebski Apr 28, 2022

Choose a reason for hiding this comment

mdjastrzebski Apr 28, 2022

Choose a reason for hiding this comment

mdjastrzebski Apr 28, 2022

Choose a reason for hiding this comment

mdjastrzebski commented May 4, 2022

kiranjd commented May 5, 2022 via email

kiranjd commented Jul 25, 2022

AugustinLF commented Jul 28, 2022

mdjastrzebski commented Aug 1, 2022

AugustinLF commented Aug 25, 2022 • edited Loading

kiranjd commented Aug 25, 2022

AugustinLF commented Sep 14, 2022

kiranjd commented Sep 15, 2022

mdjastrzebski commented Sep 16, 2022

mdjastrzebski commented Sep 22, 2022

kiranjd commented Dec 2, 2021 •

edited by thymikee

Loading

kiranjd Mar 30, 2022 •

edited

Loading

AugustinLF commented Aug 25, 2022 •

edited

Loading