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

UnderlineNav2: Take the listing html structure back to its original state #3559

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

broccolinisoup
Copy link
Member

When running integration tests at dotcom, found out that the select issue fix introduces some changes that was breaking UnderlineNav usages. (Error log (GitHub stuff only link))

It is mainly the HTML re-structure I made here.

Relying on the implementation details on the consumer side for the key prop for iteration causing some issues. I was expecting linkItem.props.children to always have a value however for custom components like below, there is no children passed. So taking the HTML structure back to its origin where we don't need to manually extract the key value from the UnderlineNav children components, so that the iteration all will be on the consumer side.

function OverviewFileTab({
  tabName,
  tabId,
  icon,
  selectedTab,
  setSelectedTab,
}: {
  tabName: string
  tabId: string
  icon: FunctionComponent<IconProps>
  selectedTab: string
  setSelectedTab: (tab: string) => void
}) {
  return (
    <UnderlineNav.Item
      as="button"
      icon={icon}
      aria-current={selectedTab === tabId ? 'page' : undefined}
      onSelect={ev => {
        setSelectedTab(tabId)
        ev.preventDefault()
      }}
      sx={{backgroundColor: 'unset', border: 'none'}}
    >
      {tabName}
    </UnderlineNav.Item>
  )
}

const licenses = [
  {
    path: 'LICENSE',
    tabName: 'License',
  },
  {
    path: 'package.json',
    tabName: 'package.json',
  },
  {
    path: 'README.md',
    tabName: 'README.md',
  },
]

export const TabTypeExample = () => {
  const [selectedTab, setSelectedTab] = React.useState('LICENSE')

  return (
    <UnderlineNav aria-label="Repository">
      {licenses.map(({tabName, path}) => (
        <OverviewFileTab
          key={path}
          tabName={tabName}
          tabId={path}
          icon={LawIcon}
          selectedTab={selectedTab}
          setSelectedTab={setSelectedTab}
        />
      ))}
    </UnderlineNav>
  )
}

Closes # (type the issue number after # if applicable; otherwise remove this line)

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@broccolinisoup broccolinisoup added the skip changeset This change does not need a changelog label Jul 25, 2023
@broccolinisoup broccolinisoup requested review from a team and joshblack July 25, 2023 03:31
@changeset-bot
Copy link

changeset-bot bot commented Jul 25, 2023

⚠️ No Changeset found

Latest commit: 76b9f06

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@broccolinisoup broccolinisoup changed the title Take the listing html structure back to its original structure UnderlineNav2: Take the listing html structure back to its original state Jul 25, 2023
@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
dist/browser.esm.js 102.41 KB (-0.01% 🔽)
dist/browser.umd.js 102.99 KB (-0.02% 🔽)

@joshblack joshblack mentioned this pull request Jul 26, 2023
4 tasks
@broccolinisoup broccolinisoup added this pull request to the merge queue Jul 27, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 27, 2023
@broccolinisoup broccolinisoup added this pull request to the merge queue Jul 27, 2023
Merged via the queue into main with commit 7716b31 Jul 27, 2023
@broccolinisoup broccolinisoup deleted the bs/underlineNav-key-fix branch July 27, 2023 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants