Update tabs API #1700
Update tabs API #1700
Conversation
intergalacticspacehighway
requested review from
dannify,
devongovett and
snowystinger
as code owners
| state: SingleSelectListState<T>, | ||
| ref: RefObject<HTMLElement> | ||
| ): TabAria { | ||
| let {item, isDisabled: propsDisabled} = props; |
There was a problem hiding this comment.
IMO it feels a bit unwieldy to have the item passed in as a prop, especially if we just need the key from it. What do you think about accepting a key instead? I figure it would be more flexible for users who consume useTab as a standalone hook
| export interface TabListState<T> extends SingleSelectListState<T> {} | ||
|
|
||
| export function useTabsState<T extends object>(props: TabsProps<T>): TabsState<T> { | ||
| export function useTabListState<T extends object>(props: TabListProps<T>): TabListState<T> { |
There was a problem hiding this comment.
Might want to rename the file to useTabListState to prevent confusion but not the biggest deal
| items?: Iterable<T>, | ||
| keyboardActivation?: 'automatic' | 'manual', | ||
| orientation?: Orientation, | ||
| isDisabled?: boolean, | ||
| disabledKeys?: Iterable<Key>, |
There was a problem hiding this comment.
Note for future work: will need to update docs and type descriptions here before releasing this update.
No need to handle in this pull, marking this as a reminder for the team
| className, | ||
| ...otherProps | ||
| } = props; | ||
| export const TabList = function <T> (props: SpectrumTabListProps<T>) { |
There was a problem hiding this comment.
Since TabList and TabPanel are now exposed to the user, I'm wondering if we wanna support any style props/UNSAFE_classNames on those components for further customization. Might not need them since we can wrap them in things like Flex/other layout components though.
| </Item> | ||
| </TabList> | ||
| <TabPanels> | ||
| <Item key="dashboard"> |
There was a problem hiding this comment.
The TabPanel items will throw a warning here since there isn't a textValue provided. I wonder if we should modify Item.ts so that a textValue isn't explicitly required since the TabList and TabPanel items don't support typeahead anyways.
|
|
||
| const {tabListProps} = useTabList({...tabProps, ...props}, state, tablistRef); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Should this be useLayoutEffect? actual question, I haven't thought it through completely
general thought is
seems like there might be a period of time where id's are being actually rendered with undefined, if we push it to useLayouEffect, then that won't actually leave the virtual dom, it'll be updated before it's flushed.
but it may not be a real issue
| let {tabListProps, tabPanelProps} = useTabs(props, state, tablistRef); | ||
| let [collapse, setCollapse] = useValueEffect(false); | ||
| let [selectedTab, setSelectedTab] = useState<HTMLElement>(); | ||
| const [tabListState, setTabListState] = React.useState<TabListState<T>>(null); |
|
Thank you!! |
4 participants
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
yarn test tabs🧢 Your Project:
GeekyAnts/NativeBase
P.S. I'll update the documentation once the changes are approved as I am not sure we need TabList and TabPanels as collection components in stately. I think React Aria users can use them as normal components and restrict them to accept Collection Item components just like Spectrum.