-
Notifications
You must be signed in to change notification settings - Fork 96
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
POC Add a trait to bootstrap ViewableData into react component #1187
base: 1
Are you sure you want to change the base?
POC Add a trait to bootstrap ViewableData into react component #1187
Conversation
@@ -100,5 +101,6 @@ export default () => { | |||
NumberField, | |||
PopoverOptionSet, | |||
ToastsContainer, | |||
CmsSiteName |
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.
This bits register our demo component in injector. Any new custom component would have to go through this step.
I nice thing here is that a developer could choose to override our component with their own custom implementation.
@@ -100,6 +100,7 @@ require('../legacy/AddToCampaignForm'); | |||
require('../legacy/SecurityAdmin'); | |||
require('../legacy/ModelAdmin'); | |||
require('../legacy/ToastsContainer'); | |||
require('../legacy/BootstrapComponent'); |
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.
We're registering tho logic to bootstrap the react components. This would only happen once in core.
import PropTypes from 'prop-types'; | ||
import classnames from 'classnames'; | ||
|
||
const CmsSiteName = ({ title, baseHref }) => ( |
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.
This replicates the existing SS templates but in JSX
import ReactDOM from 'react-dom'; | ||
import { loadComponent } from 'lib/Injector'; | ||
|
||
jQuery.entwine('ss', ($) => { |
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.
This bit uses entwine to wire our react component. All Component based off the BootstrapComponent trait would use this generic bootstrapping logic.
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.
looks good - quite similar to our implementation(below) we've got some extra handling for having an input field that the react component updates
// function to create boilerplate for standard entwine field boostrap
import jQuery from 'jquery';
import React from 'react';
import ReactDOM from 'react-dom';
import { schemaMerge } from 'lib/schemaFieldValues';
import { loadComponent } from 'lib/Injector';
export default function reactFieldBootstrapper(componentName) {
/**
* Shiv for inserting react field into entwine forms
* Also @see LeftAndMain.KeyValueField.js for reloading behaviour after form submission
*/
jQuery.entwine('ss', ($) => {
$(`.js-injector-boot .${componentName}`).entwine({
Timer: null,
Component: null,
Value: null,
Root: null,
Input: null,
setValue(value) {
this.Value = value;
const input = this.getInput();
if (input) {
input.val(value);
}
},
onmatch() {
this._super();
const cmsContent = this.closest('.cms-content').attr('id');
const context = (cmsContent)
? { context: cmsContent }
: {};
const Field = loadComponent(componentName, context);
this.setComponent(Field);
const state = this.data('state') || {};
this.setValue(state.value ? state.value : {});
const reactRoot = $(this).find('.react-holder')[0];
this.setRoot(reactRoot);
const fieldInput = $(this).find('input');
this.setInput(fieldInput);
this.refresh();
},
onunmatch() {
this._super();
// solves errors given by ReactDOM "no matched root found" error.
const container = $(this).children('.react-holder')[0];
if (container) {
ReactDOM.unmountComponentAtNode(container);
}
},
refresh() {
const props = this.getAttributes();
const $field = $(this);
const onChange = (value) => {
this.setValue(value);
// There are instances where updating the input value shouldn't
// rerender the element
if (props.dontRefresh === true) {
return;
}
this.refresh();
// Trigger change detection (see jquery.changetracker.js)
clearTimeout(this.getTimer());
const timer = setTimeout(() => {
$field.trigger('change');
}, 0);
this.setTimer(timer);
};
const Field = this.getComponent();
ReactDOM.render(
<Field
{...props}
onChange={onChange}
value={this.getValue()}
noHolder
/>,
this.getRoot()
);
},
/**
* Find the selected node and get attributes associated to attach the data to the form
*
* @returns {Object}
*/
getAttributes() {
const state = $(this).data('state');
const schema = $(this).data('schema');
return schemaMerge(schema, state);
},
});
});
}
|
||
use SilverStripe\Core\Convert; | ||
|
||
trait BootstrapComponent |
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.
This trait contains most of the logic that needs to be added to a ViewableData object to be rendered as a React component.
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.
The trait is coupled to ViewableData
, which to me implies that this should be a subclass, not a trait. What are the tradeoffs for asking devs to subclass ReactComponent
or something?
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'd say the major advantage of keeping is a trait is that developers can add the trait to a DataObject
(or any other ViewableData
subclass like FormField
or the like) if they want to.
We would probably need a slightly more specialised version of BootstrapComponent for React Form Fields, but this shows the gist of what it would look like. |
|
||
public function forTemplate() | ||
{ | ||
$return = $this->renderWith($this->getTemplates()); |
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.
might be nice to throw somewhere if we aren't on viewable data, just in case someone expects that to work
|
||
class SiteName extends ViewableData | ||
{ | ||
use BootstrapComponent; |
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.
looks nice and clean :)
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.
This is really cool. Reduces a lot of the toil around React and will definitely invite more uptake. I just have an architectural issue with the trait that could use some discussion, I think.
|
||
use SilverStripe\Core\Convert; | ||
|
||
trait BootstrapComponent |
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.
The trait is coupled to ViewableData
, which to me implies that this should be a subclass, not a trait. What are the tradeoffs for asking devs to subclass ReactComponent
or something?
I'm of two minds about the trait. In support of a trait - i can be applied to existing fields which is great for extending functionality - eg the treedropdown field or multivalue. |
Yeah, fair point. |
Yeah ... I think the main value of a trait is to be able to retroactively add it to existing classes. My thinking is to create a ReactComponent interface that dev could choose to apply to classes that should be rendered with React. The trait would mostly be a sensible implementation of that interface. It's also worth mentioning that technically the only explicit coupling to ViewableData is the call to Another side question could be do we want to use the trait approach for form fields as well? I was thinking most of those form field already have a react implementations ... we could actually put that "render in react" logic directly in Even if we decided not to flick that switch on all the time for all the fields, any React based form field would just need to extend |
1628428
to
d9edc07
Compare
This Proof of Concept showcase how we could make it easier to for developers to create PHP Objects that maps to react component.
To illustrate this we've created a SiteName ViewableData class to replace the current Site name in the top left corner of the CMS.
Update
I spun off the idea into its own repo: https://github.com/maxime-rainville/silverstripe-react/
I have a related recipe that makes it easy to use this in your own project as well: https://github.com/maxime-rainville/recipe-react/
Also have an example form field built using this concept: https://github.com/maxime-rainville/silverstripe-copy-field