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

React wrapper: Change the way editor might be passed to HotColumn and HotTable to use render props approach #10793

Conversation

NOtherDev
Copy link

Context

Specifying editors as children components marked with "magic" hot-editor attribute does not feel idiomatic. It doesn't represent something that is a part of the column, it's purely the column's property. Moreover, this kind of composition allows putting multiple Editors in one column, which is illogical.

Another issue is that a custom component needs to inherit from a specific base class, thus needs to be a class-based component, which isn't seen correct in modern React and might be hard to follow for some developers.

Instead, we're proposing to use a variant of render props pattern and allow specifying functional component-based editors via HotTable or HotColumn props directly.

Implementation notes

Well, good luck to anyone reviewing this beast.

It took me quite a lot of time to get it to the working state and I'm not extremely satisfied with the shape it takes now, to the extent that I have some doubts if it's better than the previous approach.

There are three layers of the solution: HotTable component and its editor prop, custom editor component and the dynamic class that bridges them in order to satisfy native Handsontable API. In the previous approach the editor component was required to be class-based, inheriting from a provided base class. In the new approach it needed to be functional, so bridging it to the native Handsontable API via dynamic class is more challenging.

In the discussion in #3 we figured out that useImperativeHandle might be a good fit to provide HOT editor hook functions from the inner custom editor component to the outer code (eventually to the bridging class), although during the implementation I discovered a bit more constraints/consequences of this approach:

  • HOT editor hook functions sometimes need to reference other functions or call base (super class) behaviors; while others might be called with properly bound this, there's no way to emulate super pointer easily in non-class-based code
  • there are use cases where the HOT-native editor instance needs to be accessed from within the custom editor component, while it isn't created yet at the moment the custom editor is mounted, so ref access was needed; it means there are two-way refs: custom editor sends its hook functions up via context and useImperativeHandle while it receives back the HOT-native editor instance back via another ref.
  • I ended up creating a helper hook useHotEditor that wraps the above - it hides useImperativeHandle, provides super emulation and returns editor instance ref

These together make the final user-facing API like this:

<HotTable editor={CustomEditor} ... />

or, if it needs to be customized:

<HotTable editor={() => <CustomEditor customProp={1} />} ... />
  • the editor itself:
const CustomEditor = (props: Props) => {
  const hotEditorInstanceRef = useHotEditor(ref, (runSuper) => ({
    prepare(...args) {
      runSuper().prepare(...args); // used to be super.prepare(...)
      this.otherHook(); // calls BaseEditor method
      // whatever custom code
    }
  });

 const finish = () => hotEditorInstanceRef.current.finishEditing(); // used to be this.finishEditing()

  return (
    <button onClick={finish}>test</>
  );
};

How has this been tested?

  • All the unit tests still pass + new behavior tests implemented.
  • Example projects from the repo still work properly.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

  1. Proposal in RFC
  2. Already pre-reviewed discussion with @evanSe in my private fork

Affected project(s):

  • handsontable
  • @handsontable/angular
  • @handsontable/react
  • @handsontable/vue
  • @handsontable/vue3

Checklist:

@NOtherDev
Copy link
Author

Thx @evanSe for approving. I was waiting to make it non-draft until #10791 is merged to have a clear state here 🙂

@evanSe
Copy link
Member

evanSe commented Feb 21, 2024

Thx @evanSe for approving. I was waiting to make it non-draft until #10791 is merged to have a clear state here 🙂

Ping when ready for review on this, will then merge once good!

@NOtherDev NOtherDev force-pushed the feature/improved-react-4-hoteditor branch from 73e9ef5 to 22c567f Compare February 21, 2024 13:29
@NOtherDev NOtherDev marked this pull request as ready for review February 21, 2024 13:33
@NOtherDev
Copy link
Author

Ping when ready for review on this, will then merge once good!

Here you are @evanSe 🙂

@evanSe evanSe merged commit 0377646 into handsontable:feature/improved-react Feb 21, 2024
18 of 19 checks passed
@NOtherDev NOtherDev deleted the feature/improved-react-4-hoteditor branch February 21, 2024 20:35
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.

None yet

2 participants