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

Proposal for new editors system #1166

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Proposal for new editors system #1166

wants to merge 12 commits into from

Conversation

saskliutas
Copy link
Member

@saskliutas saskliutas commented Dec 20, 2024

Proposal for new editor system

Proposal for the new editors system. This would replace existing editors system in order to simplify requirements for custom editors. Also it removes static EditorManager in favor of editors registry controlled through React context.

API overview:

  • EditorsRegistry - Application should use it to register their on custom editors or custom editors provided by some dependencies so that they would be accessible to all components in the application that uses editors system:

    • EditorsRegistryProvider - adds supplied editors to the registry hold in React context. It supports nesting multiple EditorsRegistryProvider to allow registering custom editors specific for some component that have higher priority than the ones registered near the root of the application.
    • useEditor - hook to get the editor that should be used to edit the supplied value. First it looks for applicable editor in EditorsRegistry and if none was found it fallbacks to the default editors.
  • Editor (name subject to change) - wrapper around EditorRegistry that provides a convenient way to render editors for specific value.

  • CommitingEditor (name subject to change) - wrapper around Editor that works as uncontrolled component that commits value on Enter or when editor completes value change (e.g. Toggle click). Also allows to cancel editing on Esc.

  • Value - type for all values that are supported by editors.

  • ValueMetadata - type for additional metadata that can be supplied to editors alongside value itself. It can be extended when implementing custom editors. (E.g. passing available choices and icons to the enum editor that is rendered as button group)

  • Default editors and accompanying use<EditorName>Props hooks that acts as a type guard to conveniently convert from general EditorProps to the specific editor props.

  • EditorInterop - internal implementation that maps PropertyRecord to the new Value and ValueMetadata types. It is needed to support existing usage on the editors.

Units supports

New editors system was aimed to provide better support for units. There is base component that should help with that FormattedNumericInput. It should be easy to write an editor on top of it that would know how to find Parser/Formatter for specific unit. E.g: https://github.com/iTwin/appui/tree/editors/new-system/ui/imodel-components-react/src/imodel-components-react/inputs/newEditors

Custom editors

The goal of the new editors system is to remove the need for static editor registration and provide more convenient API for implementing custom editors. Current API has quite a lot optional properties that do not make sense (propertyRecord is optional but if it is undefined there is no way to figure out what to render):

Example of custom editor using old editor system and react class components:

interface CustomBooleanEditorState {
  currentValue: boolean;
}

class CustomBooleanEditor
  extends React.PureComponent<PropertyEditorProps, CustomBooleanEditorState>
  implements TypeEditor
{
  private _inputElement = React.createRef<HTMLInputElement>();
  public override readonly state: Readonly<CustomBooleanEditorState> = {
    currentValue: false,
  };

  public async getPropertyValue(): Promise<PropertyValue | undefined> {
    // this is an optional prop for some reason.
    const record = this.props.propertyRecord;
    let propertyValue: PropertyValue | undefined;

    if (record && record.value.valueFormat === PropertyValueFormat.Primitive) {
      propertyValue = {
        valueFormat: PropertyValueFormat.Primitive,
        value: this.state.currentValue,
        displayValue: "",
      };
    }

    return propertyValue;
  }

  public get htmlElement(): HTMLElement | null {
    return this._inputElement.current;
  }

  public get hasFocus(): boolean {
    return document.activeElement === this._inputElement.current;
  }

  public override componentDidUpdate() {
    const { propertyRecord } = this.props;
    if (
      propertyRecord &&
      propertyRecord.value.valueFormat === PropertyValueFormat.Primitive
    ) {
      this.setState({ currentValue: propertyRecord.value.value as boolean });
    }
  this.setState({ currentValue: false });
  }

  public override render() {
    return (
      <ToggleSwitch
        ref={this._inputElement}
        checked={this.state.currentValue}
        onChange={(e) => {
          const newValue = e.currentTarget.checked;
          this.setState({ currentValue: newValue }, () => {
            if (!this.props.propertyRecord || !this.props.onCommit) return;
            this.props.onCommit({
              propertyRecord: this.props.propertyRecord,
              newValue: {
                valueFormat: PropertyValueFormat.Primitive,
                value: newValue,
                displayValue: "",
              },
            });
          });
        }}
      />
    );
  }
}

class CustomBooleanPropertyEditor extends PropertyEditorBase {
  public get reactNode(): React.ReactNode {
    return <CustomBooleanEditor />;
  }
}

Custom editor using old system and react functional components:

const CustomBooleanEditor = React.forwardRef<TypeEditor, PropertyEditorProps>(
  (props, ref) => {
    const inputRef = React.useRef<HTMLInputElement>(null);
    const getCurrentValue = () => {
      if (
        props.propertyRecord &&
        props.propertyRecord.value.valueFormat === PropertyValueFormat.Primitive
      ) {
        return props.propertyRecord.value.value as boolean;
      }
      return false;
    };
    const currentValue = getCurrentValue();

    React.useImperativeHandle(
      ref,
      () => ({
        getPropertyValue: async () => {
          let propertyValue: PropertyValue | undefined;
          if (
            props.propertyRecord &&
            props.propertyRecord.value.valueFormat ===
              PropertyValueFormat.Primitive
          ) {
            propertyValue = {
              valueFormat: PropertyValueFormat.Primitive,
              value: currentValue,
              displayValue: "",
            };
          }
          return propertyValue;
        },
        htmlElement: inputRef.current,
        hasFocus: document.activeElement === inputRef.current,
      }),
      [currentValue, props.propertyRecord]
    );

    return (
      <ToggleSwitch
        ref={inputRef}
        checked={currentValue}
        onChange={(e) => {
          if (!props.propertyRecord || !props.onCommit) return;
          props.onCommit({
            propertyRecord: props.propertyRecord,
            newValue: {
              valueFormat: PropertyValueFormat.Primitive,
              value: e.target.checked,
              displayValue: "",
            },
          });
        }}
      />
    );
  }
);

export class CustomBooleanPropertyEditor extends PropertyEditorBase {
  public get reactNode(): React.ReactNode {
    return <CustomBooleanEditor />;
  }
}

Custom boolean editor using new system:

function getBooleanValue(value: Value | undefined): BooleanValue {
  return value && isBooleanValue(value) ? value : { value: false };
}

export function CustomBooleanEditor(props: EditorProps) {
  // converts passed value into the boolean.
  const currentValue = getBooleanValue(props.value);

  const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
    const newValue = { value: e.target.checked };
    props.onChange(newValue);
    props.onFinish();
  };

  return <ToggleSwitch checked={currentValue.value} onChange={handleChange} />;
}

The new system removes all the code that was associated with class components and accessing values through editor ref. It is not clear if that was used/useful so the chosen approach is to add something similar later if that is still needed. Majority of that was used by EditorContainer that is represented by CommitingEditor in the new system.

TODO

  • Rewrite existing editors that support PropertyEditorParams from PropertyRecord. Need to find a way how to sunset those PropertyEditorParams in the future but in mean time if should be possible to maintain what is already there in the old system.
  • Need more work on webfont icons references by Tools in PropertyEditorParams. The initial approach is to maintains internal registry (iconName: string) => ReactNode that would hold currently used icons. Open for suggestions on this one.
  • Investigate more if current approach can be easily with unit format overrides.
  • Do we need CommitingEditor as general solutions for committing entered values only on Enter/Blur or each components should have it's own logic to handle such workflows?
  • Add visual tests.
  • Add unit tests.
  • Deprecate old editors.
  • Remove Presentation from test-app (used for debugging and testing editors feature parity)

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.

1 participant