Skip to content

fix: Tabs from testing#7463

Merged
snowystinger merged 6 commits into
mainfrom
tab-fixes-from-testing
Dec 4, 2024
Merged

fix: Tabs from testing#7463
snowystinger merged 6 commits into
mainfrom
tab-fixes-from-testing

Conversation

@snowystinger
Copy link
Copy Markdown
Member

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:

@rspbot
Copy link
Copy Markdown

rspbot commented Dec 4, 2024

@rspbot
Copy link
Copy Markdown

rspbot commented Dec 4, 2024

@rspbot
Copy link
Copy Markdown

rspbot commented Dec 4, 2024

@rspbot
Copy link
Copy Markdown

rspbot commented Dec 4, 2024

reidbarber
reidbarber previously approved these changes Dec 4, 2024
Copy link
Copy Markdown
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread packages/@react-spectrum/s2/stories/Tabs.stories.tsx
Copy link
Copy Markdown
Collaborator

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

I don't see a way to test tab collapse and icon only?

Comment thread packages/@react-spectrum/s2/stories/Tabs.stories.tsx
Comment thread packages/@react-spectrum/s2/src/TabsPicker.tsx Outdated
Comment thread packages/@react-spectrum/s2/stories/Tabs.stories.tsx
@snowystinger
Copy link
Copy Markdown
Member Author

I don't see a way to test tab collapse and icon only?

Drag the resize handle smaller on the Icons story with the isIconOnly control set to true

@rspbot
Copy link
Copy Markdown

rspbot commented Dec 4, 2024

<Tab id="FoR"><Edit /><Text>Founding of Rome</Text></Tab>
<Tab id="MaR">Monarchy and Republic</Tab>
<Tab id="Emp">Empire</Tab>
<Tab id="FoR" aria-label="Edit"><Edit /><Text>Edit</Text></Tab>
Copy link
Copy Markdown
Collaborator

@ktabors ktabors Dec 4, 2024

Choose a reason for hiding this comment

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

Should the keys change to match the new values? Why are we disabling all the keys?

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.

a bit of knowledge about the internal workings that is being tested to make sure that disabling all the keys individually works the same
this is a chromatic story, not fussed about the keys

@ktabors
Copy link
Copy Markdown
Collaborator

ktabors commented Dec 4, 2024

I don't see a way to test tab collapse and icon only?

Drag the resize handle smaller on the Icons story with the isIconOnly control set to true

On mobile the screen is wider than three iconOnly tabs so no collapse. Icon only it is currently the only story that isn't collapsed on mobile.

Retested and it's a little weird. Going to the story initially shows collapsed, set to icon only and it still shows tabs collapsed. Navigate away and come back and Tabs are no longer collapsed. Changing the prop appears to not trigger the resize observer, not sure it should, this seems like a testing only use case. With this weirdness it is possible see the icon only tabs with a picker that shows icons and text items, which is what we really care about and why I mentioned this.

@rspbot
Copy link
Copy Markdown

rspbot commented Dec 4, 2024

@rspbot
Copy link
Copy Markdown

rspbot commented Dec 4, 2024

## API Changes

@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>
-  iconOnly?: boolean
   id?: string
   isDisabled?: boolean
+  isIconOnly?: boolean
   keyboardActivation?: 'automatic' | 'manual' = 'automatic'
   onSelectionChange?: (Key) => void
   orientation?: Orientation = 'horizontal'
   selectedKey?: Key | null
   styles?: StylesPropWithHeight
 }

/@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>
-  iconOnly?: boolean
   id?: string
   isDisabled?: boolean
+  isIconOnly?: boolean
   keyboardActivation?: 'automatic' | 'manual' = 'automatic'
   onSelectionChange?: (Key) => void
   orientation?: Orientation = 'horizontal'
   selectedKey?: Key | null
   styles?: StylesPropWithHeight
 }

@snowystinger snowystinger added this pull request to the merge queue Dec 4, 2024
Merged via the queue into main with commit 494e01c Dec 4, 2024
@snowystinger snowystinger deleted the tab-fixes-from-testing branch December 4, 2024 23:13
LFDanLu added a commit that referenced this pull request Jan 11, 2025
github-merge-queue Bot pushed a commit that referenced this pull request Jan 13, 2025
* Revert "chore: Update JSDocs and apis for release (#7595)"

This reverts commit 101d077.

* Revert "fix: Tabs infinite loop (#7487)"

This reverts commit 8228e4e.

* Revert "fix: Tabs from testing (#7463)"

This reverts commit 494e01c.

* Revert "feat: Tabs collapse behaviour (#7202)"

This reverts commit cd4da2b.

* add stuff back that we still want

* update lock
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.

5 participants