Skip to content

Allow props to be passed through slots#263

Merged
devongovett merged 16 commits intomasterfrom
move-slots-out-of-style-props
Mar 13, 2020
Merged

Allow props to be passed through slots#263
devongovett merged 16 commits intomasterfrom
move-slots-out-of-style-props

Conversation

@snowystinger
Copy link
Copy Markdown
Member

Closes https://jira.corp.adobe.com/browse/RSP-1582

✅ Pull Request Checklist:

  • Included link to corresponding 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 Team:

Added functionality to Divider so it can get props from slotProps other than className
# Conflicts:
#	packages/@react-spectrum/dialog/src/AlertDialog.tsx
#	packages/@react-spectrum/dialog/src/Dialog.tsx
#	packages/@react-spectrum/dialog/stories/Dialog.stories.tsx
#	packages/@react-spectrum/dialog/stories/DialogTrigger.stories.tsx
…just convert a css module to the slots api, made useSlotProps behave like useProviderProps, added a test helper function
@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

Comment thread package.json
"@storybook/addon-knobs": "^5.2.1",
"@storybook/addon-links": "^5.2.1",
"@storybook/react": "^5.2.1",
"@testing-library/jest-dom": "^5.1.1",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

noticed this was now the package while looking up API's

import {ReactNode} from 'react';

export type Slots = {[key: string]: string};
export type Slots = {[key: string]: any};
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

unsure the better typing for this
it's really an object with anything in it, though usually with UNSAFE_className at least (but not required)


expect(result.current.toasts.length).toEqual(1);
expect(result.current.toasts[0].timer).toBe(undefined);
describe('timers', () => {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed this because jest was spitting out a terrible warning
this has influenced me to make a separate PR for others #265

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

Copy link
Copy Markdown
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.

The code changes look fine to me, but I just wanna make sure I understand the reason behind the changes.
For us:
This allows us to pass what ever props we want to slotted children components. For instance, Dialog doesn't know what order or even what children will be provided to it, but if we wanted to attach a internal ref/tracking id to the element that occupies the "hero" slot we could do so by including it in the "slots" section like so:

In Dialog.tsx:

slots = {
      container: {UNSAFE_className: styles['spectrum-Dialog-grid']},
      hero: {UNSAFE_className: styles['spectrum-Dialog-hero'], id: 'SPECIFIC ID WE WANT', OTHER STUFF},
...

For the end user:
The end user can consume the slot stuff we pass via useSlotProps in their own custom component. This means they can still place their custom component in a slot by grabbing the returned classname, and potentially use/discard the other stuff passed through the slot. Like if we had a internal ref that a parent component passes to a specific slot component, they would be able to extract that to reuse/sync with their own ref?

@snowystinger
Copy link
Copy Markdown
Member Author

@LFDanLu yes, correct, and after this PR I'll be passing an id down to the heading specifically so that we can aria-labelledby the dialog with the heading

LFDanLu
LFDanLu previously approved these changes Mar 12, 2020
@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

devongovett
devongovett previously approved these changes Mar 13, 2020
# Conflicts:
#	packages/@react-spectrum/actiongroup/src/ActionGroup.tsx
@snowystinger snowystinger dismissed stale reviews from devongovett and LFDanLu via 2bb6a7c March 13, 2020 00:33
@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@devongovett devongovett merged commit 8335514 into master Mar 13, 2020
@devongovett devongovett deleted the move-slots-out-of-style-props branch March 13, 2020 01:09
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