Skip to content

Commit 5b2c714

Browse files
graceparkgracepark
authored
Remove sidebarhomepage and add external links to More docs (github#39738)
1 parent 6c3962d commit 5b2c714

File tree

12 files changed

+54
-187
lines changed

12 files changed

+54
-187
lines changed

‎components/DefaultLayout.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ export const DefaultLayout = (props: Props) => {
113113
</a>
114114
<Header />
115115
<div className="d-lg-flex">
116-
<SidebarNav />
116+
{isHomepageVersion ? null : <SidebarNav />}
117117
{/* Need to set an explicit height for sticky elements since we also
118118
set overflow to auto */}
119119
<div className="flex-column flex-1 min-width-0">

‎components/sidebar/SidebarNav.tsx‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import cx from 'classnames'
22

33
import { useMainContext } from 'components/context/MainContext'
44
import { SidebarProduct } from 'src/landings/components/SidebarProduct'
5-
import { SidebarHomepage } from 'src/landings/components/SidebarHomepage'
65
import { AllProductsLink } from './AllProductsLink'
76
import { ApiVersionPicker } from 'src/rest/components/ApiVersionPicker'
87
import { Link } from 'components/Link'
@@ -54,11 +53,7 @@ export const SidebarNav = ({ variant = 'full' }: Props) => {
5453
style={{ width: 326, height: 'calc(100vh - 175px)', paddingBottom: sidebarPaddingBottom }}
5554
role="banner"
5655
>
57-
{!currentProduct || currentProduct.id === 'search' ? (
58-
<SidebarHomepage />
59-
) : (
60-
<SidebarProduct />
61-
)}
56+
<SidebarProduct />
6257
</div>
6358
</div>
6459
)

‎src/landings/components/ProductSelectionCard.tsx‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { useRouter } from 'next/router'
66
import { useVersion } from 'components/hooks/useVersion'
77
import { Link } from 'components/Link'
88
import * as Octicons from '@primer/octicons-react'
9+
import { LinkExternalIcon } from '@primer/octicons-react'
910

1011
type ProductSelectionCardProps = {
1112
name: string
@@ -75,6 +76,11 @@ export const ProductSelectionCard = ({ name, group }: ProductSelectionCardProps)
7576
<li key={product.name} className="pt-2">
7677
<Link href={href(product)} target={product.external ? '_blank' : undefined}>
7778
{product.name}
79+
{product.external && (
80+
<span className="ml-1">
81+
<LinkExternalIcon aria-label="(external site)" size="small" />
82+
</span>
83+
)}
7884
</Link>
7985
</li>
8086
)

‎src/landings/components/ProductSelections.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { ProductSelectionCard } from './ProductSelectionCard'
55

66
export type ProductGroupT = {
77
name: string
8-
icon: string
8+
icon?: string
99
octicon: string
1010
children: Array<ProductT>
1111
}

‎src/landings/components/SidebarHomepage.module.scss‎

Lines changed: 0 additions & 10 deletions
This file was deleted.

‎src/landings/components/SidebarHomepage.tsx‎

Lines changed: 0 additions & 45 deletions
This file was deleted.

‎src/landings/pages/home.tsx‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,38 @@ function HomePage(props: HomePageProps) {
5151
const { gettingStartedLinks, popularLinks, productGroups } = props
5252
const { t } = useTranslation(['toc'])
5353

54+
// Adding external links here due to accessibility design changes where we removed the sidebar on the homepage
55+
// docs-team 2965
56+
if (!productGroups.find((group) => group.name === 'More docs')) {
57+
productGroups.push({
58+
name: 'More docs',
59+
octicon: 'PencilIcon',
60+
children: [
61+
{
62+
id: 'electron',
63+
name: 'Electron',
64+
href: 'https://www.electronjs.org/docs/latest',
65+
versions: [],
66+
external: true,
67+
},
68+
{
69+
id: 'codeql',
70+
name: 'CodeQL',
71+
href: 'https://codeql.github.com/docs/',
72+
versions: [],
73+
external: true,
74+
},
75+
{
76+
id: 'npm',
77+
name: 'npm',
78+
href: 'https://docs.npmjs.com/',
79+
versions: [],
80+
external: true,
81+
},
82+
],
83+
})
84+
}
85+
5486
return (
5587
<div>
5688
<HomePageHero />

‎tests/rendering-fixtures/homepage.js‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,16 @@ describe('home page', () => {
1616
const main = $('[data-testid="product"]')
1717
const links = main.find('a[href*="/"]')
1818
const hrefs = links.map((i, link) => $(link)).get()
19+
let externalLinks = 0
1920
for (const href of hrefs) {
20-
const res = await get(href.attr('href'))
21-
expect(res.statusCode).toBe(200) // Not needing to redirect
22-
expect(href.text().includes('{%')).toBe(false)
21+
if (!href.attr('href').startsWith('https://')) {
22+
const res = await get(href.attr('href'))
23+
expect(res.statusCode).toBe(200) // Not needing to redirect
24+
expect(href.text().includes('{%')).toBe(false)
25+
} else {
26+
externalLinks++
27+
}
2328
}
24-
expect.assertions(hrefs.length * 2)
29+
expect.assertions((hrefs.length - externalLinks) * 2)
2530
})
2631
})

‎tests/rendering-fixtures/playwright-rendering.spec.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,8 @@ test('view the for-playwright article', async ({ page }) => {
3030
})
3131

3232
test('use sidebar to go to Hello World page', async ({ page }) => {
33-
await page.goto('/')
33+
await page.goto('/get-started')
3434

35-
await page.getByTestId('sidebar').getByText('Get started').click()
3635
await expect(page).toHaveTitle(/Getting started with HubGit/)
3736

3837
await page.getByTestId('product-sidebar').getByText('Quickstart').click()
@@ -190,8 +189,7 @@ test('navigate with side bar into article inside a map-topic inside a category',
190189
// Our TreeView sidebar only shows "2 levels". If you click and expand
191190
// the category, you'll be able to see the map-topic and the article
192191
// within.
193-
await page.goto('/')
194-
await page.getByTestId('sidebar').getByText('GitHub Actions').click()
192+
await page.goto('/actions')
195193
await page.getByTestId('sidebar').getByRole('treeitem', { name: 'Category' }).click()
196194
await page.getByText('Map & Topic').click()
197195
await page.getByLabel('<article> link').click()

‎tests/rendering-fixtures/sidebar.js‎

Lines changed: 1 addition & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,48 +1,9 @@
11
import { jest } from '@jest/globals'
22

3-
import { getDOMCached as getDOM, get } from '../helpers/e2etest.js'
3+
import { getDOMCached as getDOM } from '../helpers/e2etest.js'
44

55
describe('sidebar', () => {
66
jest.setTimeout(10 * 60 * 1000)
7-
8-
describe('home page', () => {
9-
test('includes external links', async () => {
10-
const $ = await getDOM('/')
11-
// Because we know exactly what's in the `externalProducts`
12-
// frontmatter property of tests/fixtures/content/index.md
13-
// We can predict what to expect to be present.
14-
const links = $('[data-testid=sidebar] ul a[href="https://github.com"]')
15-
expect(links.text()).toBe('GitHub itself')
16-
})
17-
18-
test('early access is never a link', async () => {
19-
const $ = await getDOM('/')
20-
const links = $('[data-testid=sidebar] a[href]')
21-
// The `content/index.md` has to list `early-access` in `children:`
22-
// or else those pages won't become part of the site tree. But
23-
// they should not actually appear in the side bar.
24-
const earlyAccessLinks = links.filter((i, link) =>
25-
$(link).attr('href').split('/').includes('early-access'),
26-
)
27-
expect(earlyAccessLinks.length).toBe(0)
28-
})
29-
30-
test('all links have Liquid evaluated', async () => {
31-
const $ = await getDOM('/')
32-
const links = $('[data-testid=sidebar] a[href]')
33-
const hrefs = links
34-
.filter((i, link) => $(link).attr('href').startsWith('/'))
35-
.map((i, link) => $(link))
36-
.get()
37-
for (const href of hrefs) {
38-
const res = await get(href.attr('href'))
39-
expect(res.statusCode).toBe(200) // Not needing to redirect
40-
expect(href.text().includes('{%')).toBe(false)
41-
}
42-
expect.assertions(hrefs.length * 2)
43-
})
44-
})
45-
467
describe('nav', () => {
478
test('top level product mentioned at top of sidebar', async () => {
489
const $ = await getDOM('/get-started')

0 commit comments

Comments
 (0)