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: Initial aria test util docs and listbox/tabs/tree utils #7145

Merged
merged 22 commits into from
Jan 13, 2025

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Oct 3, 2024

Closes

✅ 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:

🧢 Your Project:

RSP

Sorry, something went wrong.

category: Concepts
---

# Testing
Copy link
Member Author

Choose a reason for hiding this comment

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

Eventually this testing page will have more than just documenting the aria test utils

Copy link
Member

Choose a reason for hiding this comment

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

do we want to copy some of the content from the RSP page over here?

Copy link
Member Author

Choose a reason for hiding this comment

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

open to any suggestion as to the structure of this page and how in-depth the documentation should be. I figure it is easier and clearer to document each individual tester api in the RAC/RSP component page itself but happy to field opinions

Copy link
Member

Choose a reason for hiding this comment

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

makes sense. my only concern is that it makes the content of each of our component pages even longer (and they were already too long imo). something to consider for the future as we revamp the docs. e.g. should the testing library be a whole separate section of the website since it isn't really specific to react aria/RSP at all and in theory should work with any aria-compliant library?

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 makes sense, I'd would be 100% open to making these docs their own section once the utils have matured some more and we've confirmed their viability against non-RSP/RAC libraries


See below for the full definition of the `User` object.

<ClassAPI links={testUtilDocs.links} class={testUtilDocs.exports.User} />
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that PatternNames in the rendered type doesn't actually the true type, still trying to figure out how to handle this... It works just fine in the typescript
image

Copy link
Member

Choose a reason for hiding this comment

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

maybe already handled in Devon's updates to the docs from garage week?

@rspbot
Copy link

rspbot commented Oct 4, 2024

@LFDanLu LFDanLu marked this pull request as ready for review October 4, 2024 23:00
@LFDanLu
Copy link
Member Author

LFDanLu commented Nov 14, 2024

TODO: JSDOCs

@LFDanLu LFDanLu changed the title Initial aria test util docs docs: Initial aria test util docs Dec 9, 2024
@rspbot
Copy link

rspbot commented Dec 10, 2024

@rspbot
Copy link

rspbot commented Dec 10, 2024

* adding listbox test utils and clean up of other utils

* check that util works in tests

* add docs

* test listbox util get options section scoping

* tabs test utils

* add tests for tab test utils

* add docs for tabs testing

* update jsdoc and adding version badge

* pulling in tree utils from s2-treeview branch

modified some of the utils for consistency, but otherwise kept most of it the same. Changes to be discussed

* update docs and use the utils in the spectrum tests

* making things more consistent

* fix tests temporarily

* fix keyboard navigation if row is disabled
@rspbot
Copy link

rspbot commented Dec 13, 2024

@LFDanLu
Copy link
Member Author

LFDanLu commented Dec 13, 2024

If you want to look at the new utils aka tabs/listbox/tree (thanks @snowystinger!) more closely feel free to look at #7505. That PR also had a bunch of consistency updates and stuff so I just merged it in

@LFDanLu LFDanLu changed the title docs: Initial aria test util docs feat: Initial aria test util docs and listbox/tabs/tree utils Dec 13, 2024
@rspbot
Copy link

rspbot commented Jan 6, 2025

Copy link
Member

@yihuiliao yihuiliao left a comment

Choose a reason for hiding this comment

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

this looks really great! just one small thing

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

awesome stuff!

category: Concepts
---

# Testing
Copy link
Member

Choose a reason for hiding this comment

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

do we want to copy some of the content from the RSP page over here?


- [React Aria Components Tabs](Tabs.html#test-utils) and [React Spectrum Tabs](../react-spectrum/Tabs.html#test-utils)

- [React Aria Components Tree](Tree.html#test-utils) and [React Spectrum TreeView](../react-spectrum/TreeView.html#test-utils)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to copy the content of the test util section over to the RSP testing page as well? Then we could just link to the aria pages here and the rsp pages there?


## Testing

### Test utils <VersionBadge version="alpha" style={{marginLeft: 4, verticalAlign: 'bottom'}} />
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this sub-heading if that's the only one? Maybe just put the version badge next to "Testing"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mainly did this for parity with the RSP component testing pages that had non test util content separated from the test util content via the subheading, but I'm fine removing the subheading in general

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, on 2nd thought, there are components that have the "Testing" session but don't yet have test utils. Feels weird to omit the version badge but then add it when they eventually get test utils so I'm leaning towards keeping the sub header to make it obvious that the test utils themselves are in alpha

Copy link
Member

Choose a reason for hiding this comment

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

makes sense. my only concern is that it makes the content of each of our component pages even longer (and they were already too long imo). something to consider for the future as we revamp the docs. e.g. should the testing library be a whole separate section of the website since it isn't really specific to react aria/RSP at all and in theory should work with any aria-compliant library?

/**
* Returns the tree's rows if any.
*/
get rows(): HTMLElement[] {
Copy link
Member

Choose a reason for hiding this comment

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

Something to consider API-wise: some of the test utils provide getters that return rows/items/options, and some of them are methods (e.g. select/menu/listbox) I guess because they support the optional filter parameter. We could consider making these match for consistency. Also I assume we might eventually support sections in table/tree/etc. as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, this is something I've flip flopped on several times across several iterations of the test utils :/. I kinda wanna gauge how useful people even think the filter parameter is. If it turns out not to be very useful, I'm leaning towards making them all getters

Copy link
Member

Choose a reason for hiding this comment

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

I've mostly used rows.filter(...) in order to filter
is there some case that can't cover? or is it just convenience?

Copy link
Member Author

Choose a reason for hiding this comment

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

purely convenience, nothing a user couldn't do on their end

@rspbot
Copy link

rspbot commented Jan 10, 2025

@rspbot
Copy link

rspbot commented Jan 10, 2025

reidbarber
reidbarber previously approved these changes Jan 10, 2025
snowystinger
snowystinger previously approved these changes Jan 13, 2025
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

In general, nothing that I think would block a pre-release version.
Really coming together :)


See below for the full definition of the `User` object.

<ClassAPI links={testUtilDocs.links} class={testUtilDocs.exports.User} />
Copy link
Member

Choose a reason for hiding this comment

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

maybe already handled in Devon's updates to the docs from garage week?

devongovett
devongovett previously approved these changes Jan 13, 2025
## Introduction

Running automated tests on your application helps ensure that it continues to work as expected over time.
You can use testing tools like [Enzyme](https://enzymejs.github.io/enzyme/) or
Copy link
Member

Choose a reason for hiding this comment

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

We should probably remove enzyme at this point. I don't think it is maintained and doesn't work with newer versions of React.

@LFDanLu LFDanLu dismissed stale reviews from devongovett, snowystinger, and reidbarber via 1273fea January 13, 2025 19:16
@rspbot
Copy link

rspbot commented Jan 13, 2025

@rspbot
Copy link

rspbot commented Jan 13, 2025

## API Changes

react-aria-components

/react-aria-components:AutocompleteProps

-AutocompleteProps {
-  children: ReactNode
-  defaultInputValue?: string
-  filter?: (string, string) => boolean
-  inputValue?: string
-  onInputChange?: (string) => void
-  slot?: string | null
-}

@react-aria/test-utils

/@react-aria/test-utils:User

 User {
   advanceTimer: UserOpts['advanceTimer']
   constructor: (UserOpts) => void
-  createTester: (T, ObjectOptionsTypes<T>) => ObjectType<T>
-  interactionType: UserOpts['interactionType']
-  user: any
+  createTester: (T, TesterOpts<T>) => Tester<T>
+  interactionType: UserOpts['interactionType'] = mouse
 }

/@react-aria/test-utils:UserOpts

 UserOpts {
   advanceTimer?: (number) => void | Promise<unknown>
-  interactionType?: 'mouse' | 'touch' | 'keyboard'
+  interactionType?: 'mouse' | 'touch' | 'keyboard' = mouse
 }

@react-spectrum/s2

/@react-spectrum/s2:Tabs

 Tabs {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   children?: ReactNode
   defaultSelectedKey?: Key
   density?: 'compact' | 'regular' = 'regular'
   disabledKeys?: Iterable<Key>
   id?: string
   isDisabled?: boolean
+  isIconOnly?: boolean
   keyboardActivation?: 'automatic' | 'manual' = 'automatic'
-  labelBehavior?: 'show' | 'hide' = 'show'
   onSelectionChange?: (Key) => void
   orientation?: Orientation = 'horizontal'
   selectedKey?: Key | null
   slot?: string | null
 }

/@react-spectrum/s2:TabsProps

 TabsProps {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   children?: ReactNode
   defaultSelectedKey?: Key
   density?: 'compact' | 'regular' = 'regular'
   disabledKeys?: Iterable<Key>
   id?: string
   isDisabled?: boolean
+  isIconOnly?: boolean
   keyboardActivation?: 'automatic' | 'manual' = 'automatic'
-  labelBehavior?: 'show' | 'hide' = 'show'
   onSelectionChange?: (Key) => void
   orientation?: Orientation = 'horizontal'
   selectedKey?: Key | null
   slot?: string | null
 }

@react-spectrum/test-utils

/@react-spectrum/test-utils:User

 User {
   advanceTimer: UserOpts['advanceTimer']
   constructor: (UserOpts) => void
-  createTester: (T, ObjectOptionsTypes<T>) => ObjectType<T>
-  interactionType: UserOpts['interactionType']
-  user: any
+  createTester: (T, TesterOpts<T>) => Tester<T>
+  interactionType: UserOpts['interactionType'] = mouse
 }

/@react-spectrum/test-utils:UserOpts

 UserOpts {
   advanceTimer?: (number) => void | Promise<unknown>
-  interactionType?: 'mouse' | 'touch' | 'keyboard'
+  interactionType?: 'mouse' | 'touch' | 'keyboard' = mouse
 }

@LFDanLu LFDanLu added this pull request to the merge queue Jan 13, 2025
Merged via the queue into main with commit 32a9a54 Jan 13, 2025
30 checks passed
@LFDanLu LFDanLu deleted the test_util_docs branch January 13, 2025 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants