Skip to content

Detecting fillable fields#520

Merged
harbournick merged 1 commit intomainfrom
fatima_text_o
May 28, 2025
Merged

Detecting fillable fields#520
harbournick merged 1 commit intomainfrom
fatima_text_o

Conversation

@falsaadehh
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Collaborator

@harbournick harbournick left a comment

Choose a reason for hiding this comment

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

See my comment here. LMK if I can clarify anything but essentially we just need to move the logic to work inside the editor, rather than in the docx processor.

return regex.test(text);
};

export const extractFillableParts = (text) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like the idea here to process-on-import, however I think only a subset of SuperDoc customers will want to detect mustaches and convert to fields (some may not use fields at all) so this becomes an optional process (and thus should be taken out of SuperConverter which is lower-level docx converting specifically).

we already have the 'annotations: true' key in the config, we can use this to determine wether to run this at all or not. And, preferably we should do it here: https://github.com/Harbour-Enterprises/SuperDoc/blob/44a6db428cd23855a891cad50f15a79bb4acb68a/packages/super-editor/src/core/Editor.js#L1005 (post conversion, pre prose-mirror rendering)

In fact, the function will look a lot like prepareCommentsForImport() I'm guessing in this case, and we can just optionally run it if the annotations flag is on

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@artem-harbour can you review this one too? I'm curious what you think. Organizationally, something about processing text in the converter rather than strictly processing docx nodes doesn't seem quite right. Mixing of concerns here. Ultimately this could be even better as a plugin, so that even after import if a user types {{ user_name }} it can also be dynamically converted to an annotation. But I'm fine with that being some future state once we learn more about how this feature fits into SuperDoc.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with you @harbournick that the processing of text in the converter does not seem quite right.

Here are some of my thoughts:

  • This should be optional and enabled/disabled via config. This won't be used by everyone, and regex can slow down processing significantly, especially for large documents.
  • prepareDocumentForImport - this seems to be a more correct place for post processing right inside PM.

Here we can replace the necessary text with annotations or whatever we need.

Something like this.

const annotation = editor.extensionService.schema.nodes.fieldAnnotation.create({}, null, null);
const tr = editor.state.tr;
tr.replaceWith(2, 7, annotation); // replace for needed positions
editor.view.dispatch(tr);

The only thing is, we need the positions of specific words to replace, not text nodes. Here we can probably use search-replace module (it supports regex), but not sure.

  • The plugin sounds like an even smarter solution that will make replacements automatically, but I think it's not needed now and will only complicate things.

  • I also think that not every feature we need should be inside superdoc repo. Ideally, I think it should be an extension that is in the webapp and is passed to SD via the config (only the necessary changes are made to the SD to support this).

attrs.highlighted = false;
attrs.type = "text";
return {
type: 'fieldAnnotation',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fieldAnnotation - this node was not designed for this feature, but probably there will be no issue with it. @harbournick what do you think?

Copy link
Copy Markdown
Collaborator

@harbournick harbournick left a comment

Choose a reason for hiding this comment

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

LGTM

@harbournick harbournick merged commit da370da into main May 28, 2025
5 of 6 checks passed
@harbournick harbournick deleted the fatima_text_o branch May 28, 2025 21:40
* }
* ])
*/
replaceWithFieldAnnotation:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the function signature could be improved here, it's very much tied to the field name now. Although these don't necessarily have to be fields, just positions.

Example

{
  replaceWithFieldAnnotation:
    (positions) =>
    ({ editor, dispatch, state, tr }) => {
      if (!dispatch) return true;

      tr.setMeta('fieldAnnotationReplace', true);

      positions.forEach((position) => {
        let { from, to, attrs } = position;
        let { schema } = editor;

        let newPosFrom = tr.mapping.map(from);
        let newPosTo = tr.mapping.map(to);
        let node = schema.nodes[this.name].create({...attrs}, null, null);

        tr.replaceWith(newPosFrom, newPosTo, node);
      });

      return true;
  },
}

},

/**
* Replace field annotation.
Copy link
Copy Markdown
Contributor

@artem-harbour artem-harbour May 29, 2025

Choose a reason for hiding this comment

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

Replace positions with field annotations.

+@param description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants