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

Audit items for release #6248

Merged
merged 23 commits into from
Apr 26, 2024
Merged

Audit items for release #6248

merged 23 commits into from
Apr 26, 2024

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Apr 23, 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

@rspbot
Copy link

rspbot commented Apr 23, 2024

@rspbot
Copy link

rspbot commented Apr 23, 2024

@LFDanLu LFDanLu changed the title (WIP) Audit items for release Audit items for release Apr 23, 2024
@LFDanLu LFDanLu marked this pull request as ready for review April 23, 2024 18:28
Copy link
Member Author

Choose a reason for hiding this comment

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

todos in these files are carried over from moving the code around, to be handled later

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 some of these docs don't actually document onAction's usage hence why I didn't add the information detailing the inline onAction. Happy to add those sections if we want

Comment on lines 54 to 70
const focusableElements = [
'input:not([disabled]):not([type=hidden])',
'select:not([disabled])',
'textarea:not([disabled])',
'button:not([disabled])',
'a[href]',
'area[href]',
'summary',
'iframe',
'object',
'embed',
'audio[controls]',
'video[controls]',
'[contenteditable]'
];

const FOCUSABLE_ELEMENT_SELECTOR = focusableElements.join(':not([hidden]),') + ',[tabindex]:not([disabled]):not([hidden])';
Copy link
Member Author

Choose a reason for hiding this comment

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

Grabbed from FocusScope, happy to export it from that package instead but wasn't sure if we want to expose that + the Tabbable selector equivalent

Copy link
Member

Choose a reason for hiding this comment

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

maybe not export the list, but we could have export util function like isFocusable in that package that just takes a dom element and returns a boolean.

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good, will add

function DropZone(props: DropZoneProps, ref: ForwardedRef<HTMLDivElement>) {
let {isDisabled} = props;
[props, ref] = useContextProps(props, ref, DropZoneContext);
let dropzoneRef = useObjectRef(ref);
Copy link
Member Author

Choose a reason for hiding this comment

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

ref change for Typescript complaining that .current didn't exist

@@ -25,17 +25,17 @@ export interface FormProps extends SharedFormProps, DOMProps {
validationBehavior?: 'aria' | 'native'
}

export const FormValidationBehaviorContext = createContext<FormProps['validationBehavior'] | null>(null);
export const FormContext = createContext<{validationBehavior: FormProps['validationBehavior']} | null>(null);
Copy link
Member Author

Choose a reason for hiding this comment

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

for now not adding the rest of the FormProps, not sure if they would be used?

Copy link
Member

Choose a reason for hiding this comment

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

i think there was an issue or discussion about this within the last week, but I haven't managed to dig it back up

@rspbot
Copy link

rspbot commented Apr 23, 2024

@rspbot
Copy link

rspbot commented Apr 23, 2024

snowystinger
snowystinger previously approved these changes Apr 24, 2024
}
},
'aria-label': isExpanded ? stringFormatter.format('collapse') : stringFormatter.format('expand'),
// TODO: the below actually isn't enough to have keyboard navigation skip over it, we need it to be a span type button but
Copy link
Member

Choose a reason for hiding this comment

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

can we use the data-rsp-prevent-focus?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yeah, good point, now that we have that attribute I can hide the above from the tree walker I believe. Will add in followup

Copy link
Member Author

Choose a reason for hiding this comment

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

went ahead and added it to the PR actually, got rid of the element.getAttribute('aria-hidden') === 'true' from the logic in isAttributeVisible since IMO our data attribute alone should be sufficient

@@ -25,17 +25,17 @@ export interface FormProps extends SharedFormProps, DOMProps {
validationBehavior?: 'aria' | 'native'
}

export const FormValidationBehaviorContext = createContext<FormProps['validationBehavior'] | null>(null);
export const FormContext = createContext<{validationBehavior: FormProps['validationBehavior']} | null>(null);
Copy link
Member

Choose a reason for hiding this comment

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

i think there was an issue or discussion about this within the last week, but I haven't managed to dig it back up

@@ -59,7 +59,7 @@ export {TextField, TextFieldContext} from './TextField';
export {ToggleButton, ToggleButtonContext} from './ToggleButton';
export {Toolbar, ToolbarContext} from './Toolbar';
export {TooltipTrigger, Tooltip, TooltipTriggerStateContext, TooltipContext} from './Tooltip';
export {Tree, TreeItem, TreeContext, TreeItemContent, TreeStateContext} from './Tree';
export {UNSTABLE_Tree, UNSTABLE_TreeItem, UNSTABLE_TreeContext, UNSTABLE_TreeItemContent, UNSTABLE_TreeStateContext} from './Tree';
Copy link
Member

Choose a reason for hiding this comment

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

lol, just in case you didn't know, Tree is unstable right now

ktabors
ktabors previously approved these changes Apr 24, 2024
@rspbot
Copy link

rspbot commented Apr 24, 2024

@rspbot
Copy link

rspbot commented Apr 25, 2024

@rspbot
Copy link

rspbot commented Apr 25, 2024

Comment on lines +1 to +4
{
"hasActionAnnouncement": "row has action",
"hasLinkAnnouncement": "row has link: {link}"
}
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 know we discussed just add the english intl files for tree's expand/collapse strings, but are we fine doing that here for gridlist's action/link announcements (concerned since this is a full released package vs tree being alpha still)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Decision is we will leave the translations in here for now so we can get them translated by the globalization team and remove the row announcements for now. This means gridlist row's announcements still have parity with what is on prod right now so should be fine

@rspbot
Copy link

rspbot commented Apr 26, 2024

@rspbot
Copy link

rspbot commented Apr 26, 2024

@rspbot
Copy link

rspbot commented Apr 26, 2024

},
'aria-label': isExpanded ? stringFormatter.format('collapse') : stringFormatter.format('expand'),
tabIndex: isAndroid() ? -1 : null,
'data-react-aria-prevent-focus': true
Copy link
Member

Choose a reason for hiding this comment

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

Oh, we already found another use case for this huh?? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, will help simplify the table's expandable row chevron when we add that to RAC as well. I can probably combine it with preventFocusOnPress to make the chevron in RAC tree completely unfocusable outside of mobile screen readers but will dig into that more after release haha

let {node} = props;
let gridListAria = useGridListItem(props, state, ref);
let isExpanded = gridListAria.rowProps['aria-expanded'] === true;
let stringFormatter = useLocalizedStringFormatter(intlMessages, 'react-aria-components');
Copy link
Member

Choose a reason for hiding this comment

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

wrong package?

Suggested change
let stringFormatter = useLocalizedStringFormatter(intlMessages, 'react-aria-components');
let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-aria/tree');

Copy link
Member Author

Choose a reason for hiding this comment

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

ah derp, copy paste strikes again, thanks

@rspbot
Copy link

rspbot commented Apr 26, 2024

@rspbot
Copy link

rspbot commented Apr 26, 2024

…ic collections

And collection-level props for dynamic collections
@devongovett
Copy link
Member

FYI I added a slight change to the docs for onAction/isDisabled. I'd like to generally recommend the item-level props for static collections, and the collection-level ones for dynamic collections so I updated the examples to follow that pattern.

@rspbot
Copy link

rspbot commented Apr 26, 2024

@rspbot
Copy link

rspbot commented Apr 26, 2024

@rspbot
Copy link

rspbot commented Apr 26, 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' }

@react-aria/combobox

AriaComboBoxProps

 AriaComboBoxProps<T> {
   allowsCustomValue?: boolean
   defaultInputValue?: string
   defaultItems?: Iterable<T>
   inputValue?: string
   items?: Iterable<T>
   menuTrigger?: MenuTriggerAction = 'input'
   onInputChange?: (string) => void
   onOpenChange?: (boolean, MenuTriggerAction) => void
-  onSelectionChange?: (Key | null) => any
+  onSelectionChange?: (Key | null) => void
   shouldFocusWrap?: boolean
 }

@react-aria/focus

isFocusable

-
+isFocusable {
+  element: HTMLElement
+  returnVal: undefined
+}

@react-aria/tree

TreeGridListItemAria

 TreeGridListItemAria {
   descriptionProps: DOMAttributes
+  expandButtonProps: AriaButtonProps
   gridCellProps: DOMAttributes
   rowProps: DOMAttributes
 }

it changed:

  • useTreeGridListItem

@react-spectrum/combobox

Section

 SpectrumComboBoxProps<T> {
   align?: 'start' | 'end' = 'end'
   allowsCustomValue?: boolean
   defaultInputValue?: string
   defaultItems?: Iterable<T>
   direction?: 'bottom' | 'top' = 'bottom'
   formValue?: 'text' | 'key' = 'text'
   inputValue?: string
   isQuiet?: boolean
   items?: Iterable<T>
   loadingState?: LoadingState
   menuTrigger?: MenuTriggerAction = 'input'
   menuWidth?: DimensionValue
   onInputChange?: (string) => void
   onOpenChange?: (boolean, MenuTriggerAction) => void
-  onSelectionChange?: (Key | null) => any
+  onSelectionChange?: (Key | null) => void
   shouldFlip?: boolean = true
   shouldFocusWrap?: boolean
 }

@dannify dannify merged commit 9ade486 into main Apr 26, 2024
25 checks passed
@dannify dannify deleted the audit_items branch April 26, 2024 23:59
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