-
Notifications
You must be signed in to change notification settings - Fork 42
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
[Work in progress] Separation of concerns using React #117
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
{ | ||
"presets": [ | ||
"@babel/preset-env", | ||
"@babel/preset-react", | ||
"@babel/preset-typescript" | ||
], | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`Edit mode should properly render the edit form 1`] = ` | ||
<div> | ||
<form> | ||
<textarea> | ||
Arbitrary markdown | ||
</textarea> | ||
<button | ||
type="submit" | ||
> | ||
RENDER | ||
</button> | ||
, | ||
</form> | ||
</div> | ||
`; | ||
|
||
exports[`should properly render markdown 1`] = ` | ||
<div> | ||
<form> | ||
<p> | ||
Some | ||
<strong> | ||
awesome | ||
</strong> | ||
markdown | ||
</p> | ||
<button | ||
type="submit" | ||
> | ||
EDIT | ||
</button> | ||
</form> | ||
</div> | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* eslint-env jest */ | ||
|
||
/* istanbul ignore next [This is a test helper, so it doesn't need to be tested itself] */ | ||
/** | ||
* This is a workaround for a bug that will be fixed in react-dom@16.9 | ||
* | ||
* The bug results in a warning being thrown about calls not being wrapped in `act()` | ||
* when a component calls `setState` twice. | ||
* More info about the issue: https://github.com/testing-library/react-testing-library/issues/281#issuecomment-480349256 | ||
* The PR that will fix it: https://github.com/facebook/react/pull/14853 | ||
*/ | ||
export function workaroundActError () { | ||
const originalError = console.error | ||
beforeAll(() => { | ||
console.error = (...args) => { | ||
if (/Warning.*not wrapped in act/.test(args[0])) { | ||
return | ||
} | ||
originalError.call(console, ...args) | ||
} | ||
}) | ||
|
||
afterAll(() => { | ||
console.error = originalError | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import * as React from 'react' | ||
import { loadMarkdown, saveMarkdown } from './service' | ||
import { View } from './view' | ||
import { ContainerProps } from '../types' | ||
|
||
export const Container: React.FC<ContainerProps> = (props) => { | ||
const [markdown, setMarkdown] = React.useState<undefined | null | string>() | ||
|
||
React.useEffect(() => { | ||
loadMarkdown(props.store, props.subject.uri) | ||
.then((markdown) => setMarkdown(markdown)) | ||
.catch(() => setMarkdown(null)) | ||
}) | ||
|
||
if (typeof markdown === 'undefined') { | ||
return <section aria-busy={true}>Loading…</section> | ||
} | ||
if (markdown === null) { | ||
return <section>Error loading markdown :(</section> | ||
} | ||
|
||
const saveHandler = (newMarkdown: string) => saveMarkdown(props.store, props.subject.uri, newMarkdown) | ||
|
||
return ( | ||
<section> | ||
<View | ||
markdown={markdown} | ||
onSave={saveHandler} | ||
/> | ||
</section> | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import * as React from 'react' | ||
import * as ReactDOM from 'react-dom' | ||
import { PaneDefinition, NewPaneOptions } from '../types' | ||
import $rdf from 'rdflib' | ||
import solidUi from 'solid-ui' | ||
import { saveMarkdown } from './service' | ||
import { Container } from './container' | ||
|
||
const { icons, store } = solidUi | ||
|
||
export const Pane: PaneDefinition = { | ||
icon: `${icons.iconBase}noun_79217.svg`, | ||
name: 'MarkdownPane', | ||
label: (subject) => subject.uri.endsWith('.md') ? 'Handle markdown file' : null, | ||
mintNew: function (options) { | ||
const newInstance = createFileName(options) | ||
return saveMarkdown(store, newInstance.uri, '# This is your markdown file\n\nHere be stuff!') | ||
.then(() => ({ | ||
...options, | ||
newInstance | ||
})) | ||
.catch((err: any) => { | ||
console.error('Error creating new instance of markdown file', err) | ||
return options | ||
}) | ||
}, | ||
render: (subject) => { | ||
const container = document.createElement('div') | ||
ReactDOM.render(<Container store={store} subject={subject}/>, container) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very simple connection between panes and React... very interesting |
||
|
||
return container | ||
} | ||
} | ||
|
||
function createFileName (options: NewPaneOptions): $rdf.NamedNode { | ||
let uri = options.newBase | ||
if (uri.endsWith('/')) { | ||
uri = uri.slice(0, -1) + '.md' | ||
} | ||
return $rdf.sym(uri) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { IndexedFormula } from 'rdflib' | ||
|
||
export function loadMarkdown (store: IndexedFormula, uri: string): Promise<string> { | ||
return (store as any).fetcher.webOperation('GET', uri) | ||
.then((response: any) => response.responseText) | ||
} | ||
|
||
export function saveMarkdown (store: IndexedFormula, uri: string, data: string): Promise<any> { | ||
return (store as any).fetcher.webOperation('PUT', uri, { | ||
data, | ||
contentType: 'text/markdown; charset=UTF-8' | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/* eslint-env jest */ | ||
import * as React from 'react' | ||
import { | ||
render, | ||
fireEvent | ||
} from '@testing-library/react' | ||
import { View } from './view' | ||
import { workaroundActError } from './actErrorWorkaround' | ||
|
||
workaroundActError() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very nice that we can do snapshot testing! |
||
|
||
it('should properly render markdown', () => { | ||
const { container } = render(<View markdown='Some **awesome** markdown' onSave={jest.fn()}/>) | ||
|
||
expect(container).toMatchSnapshot() | ||
}) | ||
|
||
describe('Edit mode', () => { | ||
it('should properly render the edit form', () => { | ||
const { container, getByRole } = render(<View markdown='Arbitrary markdown' onSave={jest.fn()}/>) | ||
|
||
const editButton = getByRole('button') | ||
editButton.click() | ||
|
||
expect(container).toMatchSnapshot() | ||
}) | ||
|
||
it('should call the onSave handler after saving the new content', () => { | ||
const mockHandler = jest.fn().mockReturnValue(Promise.resolve()) | ||
const { getByRole, getByDisplayValue } = render(<View markdown='Arbitrary markdown' onSave={mockHandler}/>) | ||
|
||
const editButton = getByRole('button') | ||
editButton.click() | ||
|
||
const textarea = getByDisplayValue('Arbitrary markdown') | ||
fireEvent.change(textarea, { target: { value: 'Some _other_ markdown' } }) | ||
|
||
const renderButton = getByRole('button') | ||
renderButton.click() | ||
|
||
expect(mockHandler.mock.calls.length).toBe(1) | ||
expect(mockHandler.mock.calls[0][0]).toBe('Some _other_ markdown') | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import * as React from 'react' | ||
import Markdown from 'react-markdown' | ||
|
||
interface Props { | ||
markdown: string; | ||
onSave: (newMarkdown: string) => Promise<void>; | ||
} | ||
|
||
export const View: React.FC<Props> = (props) => { | ||
const [phase, setPhase] = React.useState<'saving' | 'rendering' | 'editing'>('rendering') | ||
const [rawText, setRawText] = React.useState(props.markdown) | ||
|
||
function storeMarkdown () { | ||
setPhase('saving') | ||
props.onSave(rawText).then(() => { | ||
setPhase('rendering') | ||
}) | ||
} | ||
|
||
if (phase === 'saving') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this approach better than the conditionals in my experiment - I think this is easier to read. |
||
return <span aria-busy={true}>Loading…</span> | ||
} | ||
|
||
if (phase === 'editing') { | ||
return ( | ||
<form onSubmit={(e) => { e.preventDefault(); storeMarkdown() }}> | ||
<textarea | ||
onChange={(e) => { setRawText(e.target.value) }} | ||
defaultValue={rawText}/> | ||
<button type="submit">RENDER</button>, | ||
</form> | ||
) | ||
} | ||
|
||
return ( | ||
<form onSubmit={(event) => { event.preventDefault(); setPhase('editing') }}> | ||
<Markdown source={rawText}/> | ||
<button type="submit">EDIT</button> | ||
</form> | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Props should be typed of course, but looks like a reasonable way of passing data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are typed :) The generic passed to
React.FC
(i.e.ContainerProps
) defines the types of the props. (I chose to extract them to a separate file here to encourage using the same API for all top-level containers - that would make it easier to potentially extract panes into separate apps, for example.)