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

Organise a11y queries by predicate #977

Merged

Conversation

MattAgn
Copy link
Collaborator

@MattAgn MattAgn commented May 14, 2022

Summary

This PR aims to continue solving this issue by using makeQueries for all a11yQueries.

Tasks:

  • group all a11y queries by their predicate type (byRole, byA11yState...) by using makeQueries instead of makeA11yQueries
  • split the tests for each query as well
  • remove the old makeA11yQueries helper

Warning

  • The API has changed, I aligned the error messages to the same ones used for the other (non A11y) queries

Test plan

No tests should be added since the API does not change, they only need to be moved to be grouped by predicate type

@MattAgn MattAgn force-pushed the refactor/organise-a11y-queries-by-predicate branch from 89a6c26 to 966ecaa Compare May 14, 2022 17:40
@MattAgn
Copy link
Collaborator Author

MattAgn commented May 14, 2022

Hi @mdjastrzebski, I'm mostly done with this PR, just got a couple questions for you:

  • What do you think about extracting the helpers matchString and matchObject in the helpers folder? We use them respectively 3 and 2 times
  • Now that we've split all the tests in their separate files, I think it could be nice to split them a little bit more by use case scenarios. I've done an example here. I think it makes the API and the tests clearer, what do you think?

@MattAgn MattAgn force-pushed the refactor/organise-a11y-queries-by-predicate branch from 966ecaa to 527be85 Compare May 14, 2022 17:51
@MattAgn MattAgn marked this pull request as ready for review May 18, 2022 13:35
Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Great job @MattAgn! This is exactly what I was looking for! 💯

There are some small missing parts like aliased exports, please check comments.

Re moving matchString, etc to helpers: I think it's a good idea. I'd suggest helpers/machers.ts or similar for that purpose.

Re re-organizing tests a bit: I would suggest doing it in a separate PR, in order to reduce change surface area. This PR should just split existing tests into separate files without modifications. You're welcome re-organise them a little bit more in the follow-up PR :-)

src/queries/a11yHint.ts Outdated Show resolved Hide resolved
src/queries/a11yHint.ts Outdated Show resolved Hide resolved
src/queries/a11yState.ts Show resolved Hide resolved
src/queries/a11yStates.ts Outdated Show resolved Hide resolved
src/queries/a11yStates.ts Outdated Show resolved Hide resolved
src/queries/a11yValue.ts Outdated Show resolved Hide resolved
src/queries/a11yState.ts Outdated Show resolved Hide resolved
src/within.ts Show resolved Hide resolved
src/within.ts Show resolved Hide resolved
src/queries/__tests__/a11yHint.test.tsx Outdated Show resolved Hide resolved
@MattAgn
Copy link
Collaborator Author

MattAgn commented May 19, 2022

Great job @MattAgn! This is exactly what I was looking for! 💯

There are some small missing parts like aliased exports, please check comments.

Re moving matchString, etc to helpers: I think it's a good idea. I'd suggest helpers/machers.ts or similar for that purpose.

Re re-organizing tests a bit: I would suggest doing it in a separate PR, in order to reduce change surface area. This PR should just split existing tests into separate files without modifications. You're welcome re-organise them a little bit more in the follow-up PR :-)

Thanks!

Re moving matchString, etc to helpers: got it, will do so

Re re-organizing tests a bit: sure I'll do that :)

@mdjastrzebski
Copy link
Member

@MattAgn let me know if you need any help with this PR.

@MattAgn
Copy link
Collaborator Author

MattAgn commented Jun 4, 2022

@mdjastrzebski sure thanks! for now I'm good, sorry I did not have much time to work on it the past 2 weeks, but I have some time this weekend. I'll let you know how it goes :)

@MattAgn MattAgn force-pushed the refactor/organise-a11y-queries-by-predicate branch from 527be85 to 24f5259 Compare June 4, 2022 15:33
@MattAgn MattAgn force-pushed the refactor/organise-a11y-queries-by-predicate branch from 24f5259 to 5c92cb5 Compare June 4, 2022 15:36
@mdjastrzebski mdjastrzebski changed the title Organise a11y queries by predicated [DRAFT] Organise a11y queries by predicated Jul 19, 2022
Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Looks good

@mdjastrzebski mdjastrzebski changed the title [DRAFT] Organise a11y queries by predicated Organise a11y queries by predicated Jul 19, 2022
@mdjastrzebski
Copy link
Member

Weird stuff tests work fine on my local machine but timeout on CI.

@thymikee
Copy link
Member

@mdjastrzebski Try increasing the timeout then

@mdjastrzebski mdjastrzebski changed the title Organise a11y queries by predicated [DRAFT] Organise a11y queries by predicated Jul 20, 2022
@mdjastrzebski
Copy link
Member

@thymikee actually is not ma matter of timeout, I've tried to increase that but this is not the cause. Normally tests run around 1,5 minutes (other PR runs checked in Circle CI), but for this particular PR they run most of the tests (but not all) and then Jest gets stuck, no output, etc. It will wait till Circle CI no output limit and then timeout after 10 minutes or so.

@mdjastrzebski mdjastrzebski changed the title [DRAFT] Organise a11y queries by predicated Organise a11y queries by predicated Jul 20, 2022
@thymikee thymikee changed the title Organise a11y queries by predicated Organise a11y queries by predicate Jul 20, 2022
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Awesome work @MattAgn and @mdjastrzebski 🚀

@thymikee thymikee merged commit 7431885 into callstack:main Jul 20, 2022
@MattAgn
Copy link
Collaborator Author

MattAgn commented Jul 20, 2022

@mdjastrzebski thanks for the finishing touches! :)

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.

4 participants