Skip to content
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

Draft
wants to merge 1 commit into
base: 1
Choose a base branch
from

Conversation

maxime-rainville
Copy link
Contributor

@maxime-rainville maxime-rainville commented Mar 4, 2021

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

@@ -100,5 +101,6 @@ export default () => {
NumberField,
PopoverOptionSet,
ToastsContainer,
CmsSiteName
Copy link
Contributor Author

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');
Copy link
Contributor Author

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 }) => (
Copy link
Contributor Author

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', ($) => {
Copy link
Contributor Author

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.

Copy link
Contributor

@blueo blueo Mar 4, 2021

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
Copy link
Contributor Author

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.

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?

Copy link
Member

@GuySartorelli GuySartorelli Oct 27, 2021

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.

@maxime-rainville
Copy link
Contributor Author

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());
Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks nice and clean :)

Copy link

@unclecheese unclecheese left a 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

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?

@blueo
Copy link
Contributor

blueo commented Mar 5, 2021

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.

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.

@unclecheese
Copy link

Yeah, fair point. FormField extends RequestHandler 🙄 ... maybe more composition isn't such a bad idea.

@maxime-rainville
Copy link
Contributor Author

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 renderWith(). All the Attributes and ExtraClass logic snippets was actually copied over from Form and gets duplicated across a number of other classes. I'm thinking it might actually make sense to retroactively extract those bits of code to traits and remove them form Form and the other classes in which they appear.

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 FormField and enable it with a config flag. Suddenly, most of our CMS UI could be rendered in react by the flick of a switch. I'm not actually sure if this a good idea or if it would provide any value.

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 FormField and have private static $render_in_react = true statement.

@maxime-rainville maxime-rainville force-pushed the pulls/1/react-component-bootstrap-poc branch from 1628428 to d9edc07 Compare July 20, 2021 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants