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

RAC Breadcrumbs: add support for autoFocusCurrent #6325

Merged
merged 1 commit into from
May 3, 2024

Conversation

reidbarber
Copy link
Member

@reidbarber reidbarber commented May 3, 2024

For parity with RSP breadcrumbs: https://react-spectrum.adobe.com/react-spectrum/Breadcrumbs.html#props

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Check story, docs prop table, and test.

🧢 Your Project:

);

let links = getAllByRole('link');
expect(links[2]).toHaveFocus();
Copy link
Member

Choose a reason for hiding this comment

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

oo, nice, didn't know about this matcher. we can replace expect(document.activeElement).toBe(links[2]) everywhere eventually

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be an easy ast-grep codemod:

rule:
  pattern: expect(document.activeElement).toBe($A)
fix: expect($A).toHaveFocus()

I'm actually curious if that matcher is any faster. If so, we could probably speed up our suite a good bit, as much as we check for focus.

Copy link
Member Author

@reidbarber reidbarber May 3, 2024

Choose a reason for hiding this comment

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

Just tried locally out of curiosity. Didn't clear cache or do any other isolation measures though:

Before:

Time:        192.227 s

After:

Time:        160.324 s

Copy link
Member

Choose a reason for hiding this comment

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

hmm i wouldn't think it would be faster, document.activeElement is just an access to something already up to date, and then followed by an identity check. I imagine it's what the matcher is doing behind the scenes as well. ok, yep, it is, https://github.com/testing-library/jest-dom/blob/f03a582827453de6fac4510dcf4fa3148c7ed68a/src/to-have-focus.js#L7
so I'm betting you're seeing caching of some sort

@rspbot
Copy link

rspbot commented May 3, 2024

@rspbot
Copy link

rspbot commented May 3, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@reidbarber reidbarber merged commit 122d0c8 into main May 3, 2024
25 checks passed
@reidbarber reidbarber deleted the rac-breacrumbs-autoFocusCurrent branch May 3, 2024 20:03
reidbarber added a commit that referenced this pull request May 17, 2024
reidbarber added a commit that referenced this pull request May 20, 2024
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