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

Add docs for links in collections and client side routing guide #5078

Merged
merged 10 commits into from
Sep 20, 2023

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Sep 15, 2023

Docs for #4993.

Open questions:

  • Do we want to expose RouterProvider in RSP, or add the navigate prop to Provider? Right now I also have a separate "Client side routing" guide in RSP, with content mostly copied from React Aria. If we merged with Provider, I guess we could put that in the Provider docs instead?
  • What do we add to the aria hook docs? Rendering as an actual <a> element would mean updating all of the code examples.
  • Other questions in inline comments.

@@ -178,7 +178,7 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt
}
};

let linkProps = getSyntheticLinkProps(node.props);
let linkProps = itemStates.hasAction ? getSyntheticLinkProps(node.props) : {};
Copy link
Member Author

Choose a reason for hiding this comment

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

Means data-href only applies when nothing is selected (in which case the action applies). This allows the cursor to change to pointer when it's actually a link.

// Use props instead of state here. We don't want this to change due to long press.
let selectionBehavior = props.selectionBehavior || 'toggle';
let linkBehavior = props.linkBehavior || (selectionBehavior === 'replace' ? 'action' : 'override');
if (selectionBehavior === 'toggle' && linkBehavior === 'action') {
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed a bug that prevented long press on touch to select from working properly.

@@ -72,7 +76,7 @@ export function openLink(target: HTMLAnchorElement, modifiers: Modifiers, setOpe
let {metaKey, ctrlKey, altKey, shiftKey} = modifiers;
// WebKit does not support firing click events with modifier keys, but does support keyboard events.
// https://github.com/WebKit/WebKit/blob/c03d0ac6e6db178f90923a0a63080b5ca210d25f/Source/WebCore/html/HTMLAnchorElement.cpp#L184
let event = isWebKit() && process.env.NODE_ENV !== 'test'
let event = isWebKit() && isMac() && process.env.NODE_ENV !== 'test'
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed a bug causing VoiceOver on iOS not to be able to click links.

<Item href="https://google.com/" target="_blank">Google</Item>
<Item href="https://microsoft.com/" target="_blank">Microsoft</Item>
</ListBox>
```
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 don't like this example because you probably shouldn't use a listbox if you just have a list of links that doesn't support selection. But the example where the links drive the selection state via a client side router would be much larger...


```tsx example
<Picker label="Project">
<Item href="https://example.com/" target="_blank">Create new…</Item>
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to encourage this pattern?

Copy link
Member

@snowystinger snowystinger Sep 17, 2023

Choose a reason for hiding this comment

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

I think probably not... though cat may already be out of the bag in regards to this pattern?
Maybe direct people to combobox for this?

Copy link
Member

Choose a reason for hiding this comment

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

Historically we've discouraged this kind of pattern but it does feel like a pretty common use case. I'm fine with the example as is since I've seen other teams already implement something like this with a custom dummy item.

@rspbot
Copy link

rspbot commented Sep 15, 2023

@rspbot
Copy link

rspbot commented Sep 15, 2023

@rspbot
Copy link

rspbot commented Sep 16, 2023

packages/@react-aria/menu/src/useMenuItem.ts Outdated Show resolved Hide resolved
packages/@react-aria/menu/src/useMenuItem.ts Outdated Show resolved Hide resolved
packages/@react-aria/tabs/src/useTab.ts Outdated Show resolved Hide resolved
Co-authored-by: Robert Snow <rsnow@adobe.com>
@rspbot
Copy link

rspbot commented Sep 18, 2023

@rspbot
Copy link

rspbot commented Sep 18, 2023

@rspbot
Copy link

rspbot commented Sep 19, 2023

snowystinger
snowystinger previously approved these changes Sep 19, 2023
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Verified that iPad VO and FireFox keyboard w/ Command + Enter now work. Docs contents look good to me, left an opinion on the inline question you had above. Will push small copy fix I noticed in the Client Side routing example


```tsx example
<Picker label="Project">
<Item href="https://example.com/" target="_blank">Create new…</Item>
Copy link
Member

Choose a reason for hiding this comment

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

Historically we've discouraged this kind of pattern but it does feel like a pretty common use case. I'm fine with the example as is since I've seen other teams already implement something like this with a custom dummy item.

Comment on lines +105 to +107
if (router) {
contents = <RouterProvider {...router}>{contents}</RouterProvider>;
}
Copy link
Member

Choose a reason for hiding this comment

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

Ahhh, I was previously concerned that we weren't grabbing the router from the previous Provider's context but this check should work just fine

packages/dev/docs/pages/react-aria/routing.mdx Outdated Show resolved Hide resolved
packages/dev/docs/pages/react-spectrum/routing.mdx Outdated Show resolved Hide resolved
LFDanLu
LFDanLu previously approved these changes Sep 20, 2023
@rspbot
Copy link

rspbot commented Sep 20, 2023

snowystinger
snowystinger previously approved these changes Sep 20, 2023
@rspbot
Copy link

rspbot commented Sep 20, 2023

@rspbot
Copy link

rspbot commented Sep 20, 2023

## API Changes

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

@react-aria/utils

useFormReset

-
+shouldClientNavigate {
+  link: HTMLAnchorElement
+  modifiers: Modifiers
+  returnVal: undefined
+}

@react-spectrum/provider

ProviderContext

 ProviderProps {
   breakpoints?: Breakpoints = {S:380,M:768,L:1024}
   children: ReactNode
   colorScheme?: ColorScheme
   defaultColorScheme?: ColorScheme = 'light'
   isDisabled?: boolean
   isEmphasized?: boolean
   isQuiet?: boolean
   isReadOnly?: boolean
   isRequired?: boolean
   locale?: string = 'en-US'
+  router?: Router
   scale?: Scale
   theme?: Theme
   validationState?: ValidationState
 }

@devongovett devongovett merged commit 15721e5 into main Sep 20, 2023
@devongovett devongovett deleted the link-docs branch September 20, 2023 17:54
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