-
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
Conversation
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.
A lot of improvements here, and I'm more positive to React with this. Lets try a bit more experimenting and see where they take us.
import { View } from './view' | ||
import { workaroundActError } from './actErrorWorkaround' | ||
|
||
workaroundActError() |
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.
Very nice that we can do snapshot testing!
}, | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Very simple connection between panes and React... very interesting
import { View } from './view' | ||
import { ContainerProps } from '../types' | ||
|
||
export const Container: React.FC<ContainerProps> = (props) => { |
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.)
}) | ||
} | ||
|
||
if (phase === 'saving') { |
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.
I like this approach better than the conditionals in my experiment - I think this is easier to read.
This relates to #72 |
Should keep this open until we've merged it into master. |
@Vinnl I was going to work on a markdown pane and just found this! Any reason we should not merge it? |
@angelo-v IIRC this was to prototype the use of React in solid-panes, which is quite a dependency. And the other reason would be the large number of conflicts :P |
We included react already. My activtiystreams-pane is also react-based. So this should not be an issue. |
In that case I see no reason not to merge it :) |
Ok, I will give it a try |
I moved the code to a dedicated repo https://github.com/solid/markdown-pane |
This is strongly based on #116. I created a Markdown pane with practically the same functionality and a similar architecture as in that PR, but this time using React. This primarily demonstrates that it is possible to use React in a single pane, without having to share a data model with the rest - it simply attaches to a DOM node, which is then provided to the OutlineManager.
The primary change is that there is no controller here. That said, there still is a "Container" component, which glues the data fetching to the view (ie hard to test, but logic that's not too complicated). That means that the view logic is relatively easy to test - an example is included.
I'll look at porting the TrustedApplications Pane next to get a feel for how this would work for a more involved Pane, but thought I'd submit this to give an initial view of what this might look like.
Fixes #72.