Skip to content

Commit

Permalink
Add external doc sidebar links and replace "Changelog" with one. (#1273)
Browse files Browse the repository at this point in the history
* Add external doc sidebar links and replace "Changelog" with one.

Keep in mind that this is meant as a quick fix, and the current sidebar system
is likely to be overhauled in the future in a way that will have the
functionality, but not necessarily invoked with the same "type" syntax.

This branch allows the sidebar to have external links, and changes the
"Changelog" link at the end to be a link to the DVC GitHub.

Both the sidebar utils and component had to be changed to enable this- the
sidebar utils moreso as the "external" type bypasses previous logic entirely.
Anything without "external" set on it is treated the same, however.

The component change just treats external-typed entries a little differently by
setting `target=_blank` and preventing the `href` from having anything prepended
to it. The best solution is to move this out of the component level entirely,
but the best solution isn't the quickest and I've already mentioned big sidebar
changes are coming soon.

* Remove `target: null` from non-external links.

My mistake, I thought `null` was the proper syntax here and I guess the linter
missed it.

* Change existing changelog link on Community to point to releases

* Change sidebar link to releases page

* Change external link component choice to an if statement.

This is easier to read for most people, and also works better with linters.

* Make sidebar "type" type field optional

* Add an external link icon ID to the changelog sidebar item

I just grabbed an SVG we already had. It could use some work, but this is a
simple working implementation so it's worth pushing.

* Use SVGR for sidebar external icon

* Let findChildWithSource accept external links

Some parts of the application still use the sidebar.json helpers to determine
their links, and when attempting to do so on external-type links broke the
build.

This change fixes that situation, letting the `findChildWithSource` function
adapt to input that isn't internal.

It's a bit ugly, but this whole branch is a bit ugly. The sidebar overhaul
should take care of this more cleanly in the future.

* Remove old changelog pages

I'm sorry, they look like they took some effort to write! But that's also the
reason we have to stop writing these and get rid of the old ones.

* Change sidebar external icon link to match font color.

I made a new SVG based off the other one. It uses "currentColor" so it will take
on the color of its surrounding font by default, and also is fully transparent.

This new SVG is only used in the sidebar- other external link icons are still
the old hard-coded blue and white one.

* Change external link url to be under `url` key

* Add `target="_blank"` on externals by default.

This is done the same way as we already do `rel="noopener noreferrer"`.
The change applies site-wide, but only to external links that don't specify a target.

`target` can still be overridden with anything, including a blank string.

* Fix tests for new temporary sidebar logic

* Move new icon to same dir as sidebar menu component
  • Loading branch information
rogermparent authored May 9, 2020
1 parent b716311 commit 26130d4
Show file tree
Hide file tree
Showing 9 changed files with 103 additions and 180 deletions.
48 changes: 0 additions & 48 deletions content/docs/changelog/0.18.md

This file was deleted.

79 changes: 0 additions & 79 deletions content/docs/changelog/0.35.md

This file was deleted.

15 changes: 3 additions & 12 deletions content/docs/sidebar.json
Original file line number Diff line number Diff line change
Expand Up @@ -440,17 +440,8 @@
]
},
{
"slug": "changelog",
"source": false,
"children": [
{
"label": "v0.12 - v0.18",
"slug": "0.18"
},
{
"label": "v0.19 - v0.35",
"slug": "0.35"
}
]
"label": "Changelog",
"url": "https://github.com/iterative/dvc/releases",
"type": "external"
}
]
2 changes: 1 addition & 1 deletion src/components/Community/data.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
"description": "Step-by-step introduction to basic DVC features"
},
{
"url": "/doc/changelog",
"url": "https://github.com/iterative/dvc/releases",
"title": "Changelog",
"description": "See what's new in DVC."
}
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
38 changes: 29 additions & 9 deletions src/components/Documentation/Layout/SidebarMenu/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import includes from 'lodash/includes'
import ShowOnly from '../../../ShowOnly'
import DownloadButton from '../../../DownloadButton'
import Link from '../../../Link'
import { ReactComponent as ExternalLinkIcon } from './external-link-icon.svg'

import {
structure,
Expand All @@ -25,34 +26,53 @@ interface ISidebarMenuItemProps {
source: boolean | string
onClick: (e: React.MouseEvent) => void
activePaths?: Array<string>
type?: string
}

const SidebarMenuItem: React.FC<ISidebarMenuItemProps> = ({
children,
label,
path,
activePaths,
onClick
onClick,
type
}) => {
const isActive = activePaths && includes(activePaths, path)
const isRootParent =
activePaths && activePaths.length > 1 && activePaths[0] === path

return (
<>
const className = cn(
styles.sectionLink,
isActive && styles.active,
isRootParent && 'docSearch-lvl0',
'link-with-focus'
)

const parentElement =
type === 'external' ? (
<Link
href={path}
id={path}
className={className}
onClick={onClick}
target="_blank"
>
{label} <ExternalLinkIcon />
</Link>
) : (
<Link
href={getPathWithSource(path)}
id={path}
className={cn(
styles.sectionLink,
isActive && styles.active,
isRootParent && 'docSearch-lvl0',
'link-with-focus'
)}
className={className}
onClick={onClick}
>
{label}
</Link>
)

return (
<>
{parentElement}
{children && (
<Collapse isOpened={!!isActive}>
{children.map(item => (
Expand Down
15 changes: 11 additions & 4 deletions src/components/Link/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@ const ResultLinkComponent: React.FC<ILinkProps> = ({
href,
children,
rel,
target,
...restProps
}) => {
// Handle all situations where a basic `a` must be used over Gatsby Link
const hrefIsRelative = isRelative(href)
const hrefIsMailto = isMailto(href)
const hrefHasTarget = restProps.target
const hrefHasTarget = typeof target === 'string'
// Fragments within the page should be `a`, but links to other pages
// that have anchors should be okay.
const hrefIsRelativeFragment = href.startsWith('#')
Expand All @@ -40,13 +41,19 @@ const ResultLinkComponent: React.FC<ILinkProps> = ({
/*
Change external links without an explicit rel to have 'noopener
noreferrer', but leave explicitly defined rels alone.
Do the same with `target=_blank`
*/
if (!hrefIsRelative && typeof rel !== 'string') {
rel = 'noopener noreferrer'
if (!hrefIsRelative) {
if (typeof rel !== 'string') {
rel = 'noopener noreferrer'
}
if (!hrefHasTarget) {
target = '_blank'
}
}

return (
<a href={href} rel={rel} {...restProps}>
<a href={href} rel={rel} target={target} {...restProps}>
{children}
</a>
)
Expand Down
69 changes: 45 additions & 24 deletions src/utils/shared/sidebar.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,25 @@ const PATH_ROOT = '/doc'
const FILE_ROOT = '/docs/'
const FILE_EXTENSION = '.md'

function validateRawItem({ slug, source, children }) {
function validateRawItem({ slug, source, children, type, url }) {
const isSourceDisabled = source === false

if (typeof slug !== 'string') {
throw Error("'slug' field is required in objects in sidebar.json")
}
switch (type) {
case 'external':
if (typeof url !== 'string') {
throw Error("'url' field is required in external sidebar.json entries")
}
break
default:
if (typeof slug !== 'string') {
throw Error("'slug' field is required in local sidebar.json entries")
}

if (isSourceDisabled && (!children || !children.length)) {
throw Error(
"If you set 'source' to false, you had to add at least one child"
)
if (isSourceDisabled && (!children || !children.length)) {
throw Error(
'Local sidebar.json entries with no source must have children'
)
}
}
}

Expand Down Expand Up @@ -69,26 +77,35 @@ function findPrevItemWithSource(data, item) {
function normalizeItem({ rawItem, parentPath, resultRef, prevRef }) {
validateRawItem(rawItem)

const { label, slug, source, tutorials } = rawItem
const { label, slug, source, tutorials, type, url } = rawItem

// If prev item doesn't have source we need to search for it
const prevItemWithSource =
prevRef && findPrevItemWithSource(resultRef, prevRef)
switch (type) {
case 'external':
return {
type,
path: url,
label
}
default:
// If prev item doesn't have source we need to search for it
const prevItemWithSource =
prevRef && findPrevItemWithSource(resultRef, prevRef)

const prev = prevItemWithSource && prevItemWithSource.path
const prev = prevItemWithSource && prevItemWithSource.path

const sourceFileName = source ? source : slug + FILE_EXTENSION
const sourcePath = FILE_ROOT + parentPath + sourceFileName
const sourceFileName = source ? source : slug + FILE_EXTENSION
const sourcePath = FILE_ROOT + parentPath + sourceFileName

const relativePath = parentPath + slug
const relativePath = parentPath + slug

return {
path: relativePath ? `${PATH_ROOT}/${relativePath}` : PATH_ROOT,
source: source === false ? false : sourcePath,
label: label ? label : startCase(slug),
tutorials: tutorials || {},
prev,
next: undefined
return {
path: relativePath ? `${PATH_ROOT}/${relativePath}` : PATH_ROOT,
source: source === false ? false : sourcePath,
label: label ? label : startCase(slug),
tutorials: tutorials || {},
prev,
next: undefined
}
}
}

Expand Down Expand Up @@ -145,7 +162,11 @@ const normalizedSidebar = normalizeSidebar({
})

function findChildWithSource(item) {
return item.source ? item : findChildWithSource(item.children[0])
// Return item unchanged if isn't root-relative
if (!item.path.startsWith('/')) return item
return item.source
? item
: findChildWithSource(item.children && item.children[0])
}

function getFirstPage() {
Expand Down
Loading

0 comments on commit 26130d4

Please sign in to comment.