Skip to content

Commit 218c866

Browse files
peterbersese
andauthored
Enable hover preview cards for all docsets (github#36078)
Co-authored-by: Robert Sese <[email protected]>
1 parent 2606caf commit 218c866

File tree

3 files changed

+15
-76
lines changed

3 files changed

+15
-76
lines changed

Diff for: components/LinkPreviewPopover.tsx

+11-17
Original file line numberDiff line numberDiff line change
@@ -112,19 +112,6 @@ function popoverWrap(element: HTMLLinkElement) {
112112
let anchor = ''
113113

114114
if (!title) {
115-
// TEMPORARY
116-
// We're currently only activating this functionalty on a subset of pages.
117-
// In fact, only on /$locale/pages/
118-
// On the server-side, we decide this by setting or not setting the
119-
// data attributes on the tags. But for in-page anchor links we don't
120-
// rely on the server.
121-
// We can remove this if statement once preview hover cards are
122-
// enabled everywhere.
123-
const pathnameSplit = new URL(element.href).pathname.split('/')
124-
// Check for both when you're on free-pro-team@latest and any
125-
// other version too.
126-
if (!(pathnameSplit[2] === 'pages' || pathnameSplit[3] === 'pages')) return
127-
128115
// But, is it an in-page anchor link? If so, get the title, intro
129116
// and product from within the DOM. But only if we can use the anchor
130117
// destination to find a DOM node that has text.
@@ -307,14 +294,21 @@ function popoverHide() {
307294
}
308295

309296
function testTarget(target: HTMLLinkElement) {
310-
// Return true if the element is an A tag, whose `href` starts with
311-
// a `/`, is contained in either the article-contents (the meat of the article)
312-
// or the article-intro (which contain product callouts), and it's not one of
313-
// those permalink ones next to headings (with the chain looking icon).
297+
// Return true if:
298+
//
299+
// * the element is an A tag
300+
// * whose `href` starts with a `/`
301+
// * is contained in either the article-contents (the meat of the article)
302+
// or the article-intro (which contain product callouts)
303+
// * the window width is > 767px, hovercards are less useful at the smaller
304+
// widths
305+
// * it's not one of those permalink ones next to headings (with the chain
306+
// looking icon).
314307
return (
315308
target.tagName === 'A' &&
316309
target.href.startsWith(window.location.origin) &&
317310
target.closest('#article-contents, #article-intro') &&
311+
window.innerWidth > 767 &&
318312
!target.classList.contains('doctocat-link')
319313
)
320314
}

Diff for: components/article/ArticlePage.tsx

+2-40
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
import { useRouter } from 'next/router'
22
import dynamic from 'next/dynamic'
33
import cx from 'classnames'
4-
import { Box, Flash } from '@primer/react'
5-
import { LinkExternalIcon, BeakerIcon } from '@primer/octicons-react'
4+
import { LinkExternalIcon } from '@primer/octicons-react'
65

76
import { Callout } from 'components/ui/Callout'
87
import { DefaultLayout } from 'components/DefaultLayout'
@@ -64,44 +63,7 @@ export const ArticlePage = () => {
6463
<Breadcrumbs />
6564
</div>
6665
<ArticleGridLayout
67-
topper={
68-
<>
69-
{/* This is a temporary thing for the duration of the
70-
feature-flagged release of hover preview cards on /$local/pages/
71-
articles.
72-
Delete this whole thing when hover preview cards is
73-
available on all articles independent of path.
74-
*/}
75-
{router.query.productId === 'pages' && (
76-
<Flash variant="default" className="mb-3">
77-
<Box sx={{ display: 'flex' }}>
78-
<Box
79-
sx={{
80-
p: 1,
81-
textAlign: 'center',
82-
}}
83-
>
84-
<BeakerIcon className="mr-2 color-fg-muted" />
85-
</Box>
86-
<Box
87-
sx={{
88-
flexGrow: 1,
89-
p: 0,
90-
}}
91-
>
92-
<p>
93-
Hover over a link to another article to get more details. If you have ideas
94-
for how we can improve this page, let us know in the{' '}
95-
<a href="https://github.com/github/docs/discussions/24591">discussion</a>.
96-
</p>
97-
</Box>
98-
</Box>
99-
</Flash>
100-
)}
101-
102-
<ArticleTitle>{title}</ArticleTitle>
103-
</>
104-
}
66+
topper={<ArticleTitle>{title}</ArticleTitle>}
10567
intro={
10668
<>
10769
{intro && (

Diff for: lib/render-content/plugins/rewrite-local-links.js

+2-19
Original file line numberDiff line numberDiff line change
@@ -24,21 +24,6 @@ const AUTOTITLE = /^\s*AUTOTITLE\s*$/
2424
// which we use to know that we need to fall back to English.
2525
export class TitleFromAutotitleError extends Error {}
2626

27-
// E.g. `/en/pages/foo` or `/pt/pages/any/thing` or
28-
// `/en/[email protected]/pages/foo`.
29-
// But not `/en/pages` because that's not an article page anyway.
30-
const ENABLED_PATHS_REGEX = /^\/\w{2}\/pages\/|^\/\w{2}\/[\w-]+@[\w.]+\/pages\//
31-
32-
// Return true if we should bother setting data attributes.
33-
// This becomes our feature-flag switch for enabling/disabling the
34-
// hover preview cards.
35-
// If you're on a page where we don't want hover preview cards, we
36-
// can just omit these data attributes then the client-side will
37-
// simply not be able to display them because the data isn't there.
38-
function setDataAttributesOnCurrentPath(path) {
39-
return ENABLED_PATHS_REGEX.test(path)
40-
}
41-
4227
// Matches any <a> tags with an href that starts with `/`
4328
const matcherInternalLinks = (node) =>
4429
node.type === 'element' &&
@@ -57,7 +42,7 @@ const matcherAnchorLinks = (node) =>
5742
// Content authors write links like `/some/article/path`, but they need to be
5843
// rewritten on the fly to match the current language and page version
5944
export default function rewriteLocalLinks(context) {
60-
const { currentLanguage, currentVersion, currentPath } = context
45+
const { currentLanguage, currentVersion } = context
6146
// There's no languageCode or version passed, so nothing to do
6247
if (!currentLanguage || !currentVersion) return
6348

@@ -68,9 +53,7 @@ export default function rewriteLocalLinks(context) {
6853
const newHref = getNewHref(node, currentLanguage, currentVersion)
6954
if (newHref) {
7055
node.properties.href = newHref
71-
if (setDataAttributesOnCurrentPath(currentPath)) {
72-
promises.push(dataAttributesSetter(node, context))
73-
}
56+
promises.push(dataAttributesSetter(node, context))
7457
}
7558
for (const child of node.children) {
7659
if (child.value) {

0 commit comments

Comments
 (0)