Skip to content

Commit

Permalink
Fix rendering of file: links (200ok-ch#311)
Browse files Browse the repository at this point in the history
The new logic for rendering file: links introduced by 200ok-ch#307 had a
number of issues:

- It created links for absolute paths, which does not make sense
  since there is no fully general mapping between the filesystem
  and any of the sync backends.

- It created links for relative paths pointing outside the
  backend (e.g. ../../../../foo.org),

- It used <a href="..."> rather than <Link to=...>, which meant
  the PWA treated them as external links.

So create a <Link> for relative file: paths pointing to .org files,
don't link other file: paths, and render all other links with <a ...>
as before.

Fixes 200ok-ch#311.
  • Loading branch information
Adam Spiers committed Jun 2, 2020
1 parent ab3be82 commit 924a3a1
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 30 deletions.
26 changes: 16 additions & 10 deletions src/components/OrgFile/OrgFile.integration.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -462,16 +462,22 @@ describe('Render all views', () => {
expect(elem[0]).toHaveTextContent('https://foo.bar.baz/xyz?a=b&d#foo');
});

test('recognizes file: links', () => {
fireEvent.click(
queryByText('A header with a link to a local .org file as content')
);
const elem = getAllByText('a local .org file');
// There's exactly one such URL
expect(elem.length).toEqual(1);
// And it renders as such
expect(elem[0]).toHaveAttribute('href', 'schedule_and_timestamps.org');
expect(elem[0]).toHaveTextContent('a local .org file');
describe('recognizes file: links', () => {
beforeEach(() => {
fireEvent.click(
queryByText('A header with various links as content')
);
});

test('relative link to .org file', () => {
const elem = getAllByText('an existing .org file in the same directory');
// There's exactly one such URL
console.log(elem);
expect(elem.length).toEqual(1);
// And it renders as such
expect(elem[0]).toHaveAttribute('href', 'schedule_and_timestamps.org');
expect(elem[0]).toHaveTextContent('a local .org file');
});
});

test('recognizes email addresses', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ exports[`Render all views Org Functionality Renders everything starting from an
style="word-break: break-word;"
>
<span>
A header with a link to a local .org file as content
A header with various links as content
</span>
...
</span>
Expand Down
48 changes: 35 additions & 13 deletions src/components/OrgFile/components/AttributedString/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import React from 'react';

import { Link, useLocation } from 'react-router-dom';

import './stylesheet.css';

import TablePart from './components/TablePart';
Expand All @@ -11,26 +13,46 @@ import classNames from 'classnames';
export default ({ parts, subPartDataAndHandlers }) => {
let className;

let location = useLocation();

const renderLink = (part) => {
const id = part.get('id');
const uri = part.getIn(['contents', 'uri']);
const title = part.getIn(['contents', 'title']) || uri;
if (uri.startsWith("file:")) {
const target = uri.substr(5);
const isRelativeOrgFileLink =
!target.startsWith("/") &&
!target.startsWith("~") &&
!target.startsWith("../") &&
uri.endsWith(".org");
if (isRelativeOrgFileLink) {
const dir = location.pathname.match(/.*\//);
return (
<Link key={id} to={dir + target}>
{title}
</Link>
);
} else {
return title;
}
} else {
return (
<a key={id} href={uri} target="_blank" rel="noopener noreferrer">
{title}
</a>
);
}
};

return (
<span>
{parts.map((part) => {
switch (part.get('type')) {
case 'text':
return part.get('contents');
case 'link':
const uri = part.getIn(['contents', 'uri']);
const title = part.getIn(['contents', 'title']) || uri;
const isFileLink = uri.startsWith("file:");
return (
<a
key={part.get('id')}
href={isFileLink ? uri.substr(5) : uri}
target={isFileLink ? "" : "_blank"}
rel="noopener noreferrer"
>
{title}
</a>
);
return renderLink(part);
case 'percentage-cookie':
className = classNames('attributed-string__cookie-part', {
'attributed-string__cookie-part--complete':
Expand Down
7 changes: 6 additions & 1 deletion src/components/OrgFile/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,16 @@ class OrgFile extends PureComponent {
}
}

componentDidUpdate() {
componentDidUpdate(prevProps) {
const { headers, pendingCapture } = this.props;
if (!!pendingCapture && !!headers && headers.size > 0) {
this.props.org.insertPendingCapture();
}

const { path } = this.props;
if (!_.isEmpty(path) && path !== prevProps.path) {
this.props.syncBackend.downloadFile(path);
}
}

componentDidCatch(error) {
Expand Down
4 changes: 2 additions & 2 deletions src/reducers/org.unit.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ describe('org reducer', () => {
['A repeating todo', 2],
['A header with tags ', 1],
['A header with [[https://organice.200ok.ch][a link]]', 1],
['A header with a link to a local .org file as content', 1],
['A header with various links as content', 1],
['A header with a URL, mail address and phone number as content', 1],
['PROJECT Foo', 2],
["A headline that's done since a loong time", 3],
Expand All @@ -89,7 +89,7 @@ describe('org reducer', () => {
['A repeating todo', 2],
['A header with tags ', 1],
['A header with [[https://organice.200ok.ch][a link]]', 1],
['A header with a link to a local .org file as content', 1],
['A header with various links as content', 1],
['A header with a URL, mail address and phone number as content', 1],
['A header with a custom todo sequence in DONE state', 1],
]);
Expand Down
8 changes: 5 additions & 3 deletions test_helpers/fixtures/main_test_file.org
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ Some description content

* A header with tags :tag1:tag2:
* A header with [[https://organice.200ok.ch][a link]]
* A header with a link to a local .org file as content

[[file:schedule_and_timestamps.org][a local .org file]]
* A header with various links as content
Relative link to [[file:schedule_and_timestamps.org][an existing .org file in the same directory]]
Relative link to [[file:subdir/foo.org][a fictitious .org file in a sub-directory]]
Absolute link to [[file:/foo/bar/baz.org][a fictitious .org file in home directory]]
Absolute link to [[file:/foo/bar/baz.org][a fictitious .org file]]
* A header with a URL, mail address and phone number as content

This is a URL https://foo.bar.baz/xyz?a=b&d#foo in a line of text.
Expand Down

0 comments on commit 924a3a1

Please sign in to comment.