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.

Sign up for GitHub

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

Jump to bottom

feat: add Form Annotation support #2013

Closed
wants to merge 5 commits into from
Closed

feat: add Form Annotation support #2013

wants to merge 5 commits into from

Conversation

axel7083
Copy link

@axel7083 axel7083 commented Sep 8, 2022 •
edited
Loading

Since pdfkit support Form Annotation I added the logic to support them in react-pdf. I've never contributed to this project, so I am not aware if I was suppose to add this feature in the way I did. I also added the types support for TS.

See issue #42

Here is an example

<Document>
    <Page>
      <View style={{backgroundColor: 'rgba(182,28,28,0.62)', width: '30%', height: '100%'}}>
          <Form>
              <FormField name="user-info" style={{flexDirection: 'column'}}>
                  <Text>FormText</Text>
                  <FormText name={'name'} value={"hello"} align={'center'} style={{height: '50px'}} password />

                  <Text>FormCombo</Text>
                  <FormCombo
                      name={'combo'}
                      select={['', 'option 1', 'option 2']}
                      value={''}
                      defaultValue={''}
                      style={{height: '20px'}}
                  />

                  <Text>FormList</Text>
                  <FormList name={'list'} select={['', 'option 1', 'option 2']} value={''} defaultValue={''} style={{height: '50px'}}/>

                  <Text>FormPushButton</Text>
                  <FormPushButton name={'bouton'} label={'push button'} style={{height: '50px'}}/>
              </FormField>
          </Form>
      </View>
    </Page>
  </Document>

And the result example.pdf

@jamesmfriedman
Copy link

I just started looking into React-PDF today and was hoping they would add form support. Your timing is impeccable.

@diegomura diegomura changed the title Adding Form Annotation support feat: add Form Annotation support Nov 7, 2022
diegomura
diegomura reviewed Nov 7, 2022
View reviewed changes
Copy link
Owner

@diegomura diegomura left a comment

Choose a reason for hiding this comment

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

Thanks for this awesome contribution! Sorry for the late response. It's looking good, but have some questions or suggestions I would like to hear about before considering merging it or working further

packages/render/src/primitives/renderNode.js

const isRecursiveNode = node => node.type !== P.Text && node.type !== P.Svg;
const isRecursiveNode = node => node.type !== P.Text && node.type !== P.Svg && node.type !== P.Form && node.type !== P.FormField;
Copy link
Owner

Choose a reason for hiding this comment

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

Why Form and FormField aren't recursive? This creates dependency import cycles between renderForm and renderFormField and renderNode

packages/primitives/src/index.js
@@ -8,6 +8,12 @@ export const Note = 'NOTE';
export const Path = 'PATH';
export const Rect = 'RECT';
export const Line = 'LINE';
export const Form = 'FORM';
export const FormField = 'FORM_FIELD';
export const FormText = 'FORM_TEXT';
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think on renaming this to TextInput to match react native primitives?

packages/primitives/src/index.js
@@ -8,6 +8,12 @@ export const Note = 'NOTE';
export const Path = 'PATH';
export const Rect = 'RECT';
export const Line = 'LINE';
export const Form = 'FORM';
Copy link
Owner

Choose a reason for hiding this comment

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

Is this necessary? From pdfkit docs, initForm only needs to be called once. I tested this code and trying to render 2 forms crashes rendering. Maybe we can call it once globally if we detect any form elements

packages/primitives/src/index.js
export const FormField = 'FORM_FIELD';
export const FormText = 'FORM_TEXT';
export const FormPushButton = 'FORM_PUSH_BUTTON';
export const FormCombo = 'FORM_COMBO';
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about renaming this to Picker to match react native (web) primitives?

packages/primitives/src/index.js
export const Form = 'FORM';
export const FormField = 'FORM_FIELD';
export const FormText = 'FORM_TEXT';
export const FormPushButton = 'FORM_PUSH_BUTTON';
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about renaming this to Button to match react native (web) primitives? Not sure either what purpose this element has

packages/primitives/src/index.js
@@ -8,6 +8,12 @@ export const Note = 'NOTE';
export const Path = 'PATH';
export const Rect = 'RECT';
export const Line = 'LINE';
export const Form = 'FORM';
export const FormField = 'FORM_FIELD';
Copy link
Owner

Choose a reason for hiding this comment

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

Is this necessary? I get how you can create hierarchical structures but wonder it it's useful at all to expose this

diegomura
diegomura reviewed Nov 7, 2022
View reviewed changes
packages/render/src/utils/parseFormOptions.js
Comment on lines +55 to +59
export {
parseTextFieldOptions,
parseComboAndListFieldOptions,
parseButtonFieldOptions,
};
Copy link
Owner

Choose a reason for hiding this comment

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

No need to be in an utils file. I think it's better for each helper fn to be closer to their element (button, combo, text, etc)

@Ovyerus
Copy link

Ovyerus commented May 16, 2023

Hi, any chance of this being fixed up & merged in the near future? Been using react-pdf at work and we're looking to add some form stuff to our PDFs. I'm down to contribute and help get this out the door if necessary!

@axel7083
Copy link
Author

Hello @Ovyerus sadly I do not have much time to spend on side project currently, but I would be accepting contributions!

@Ovyerus
Copy link

Ovyerus commented May 16, 2023

Sounds good!

@mdefeche
Copy link

mdefeche commented Jun 5, 2023

Would love to have this feature as well !

@majid701
Copy link

@diegomura Any ETA on this. Would love this feature!

@runelk
Copy link

runelk commented Jun 13, 2023

@axel7083 @diegomura Hi, I'll try to do some work on this onwards. I have a WIP with the latest changes and @axel7083 's commits, and have started implementing some of your proposed changes @diegomura . Should I open a new PR here for this when I'm closer to done, or is it better that I make a PR against the fork?

@axel7083
Copy link
Author

@axel7083 @diegomura Hi, I'll try to do some work on this onwards. I have a WIP with the latest changes and @axel7083 's commits, and have started implementing some of your proposed changes @diegomura . Should I open a new PR here for this when I'm closer to done, or is it better that I make a PR against the fork?

If you make a PR against the fork I can quickly review it to commit it here.

@joneldiablo
Copy link

Its there documentation about form elements?

@kjossendal
Copy link

Any update on this? Or maybe someone could publish a fork while this is waiting to be merged? I would but I'm not familiar with the packaging/publishing process on a library like this.

@kjossendal
Copy link

kjossendal commented Mar 15, 2024 •
edited
Loading

Is it acceptable, when using <Form /> the way this pr does, to just ignore it's initialization if it has already been initialized? Otherwise I'm not sure where it would be done globally. For example, the below prevents broken rendering issues from misuse in pdfkit/src/mixins/acroform.js. The problem is that, with this pr implementation, <Form /> can only be used once but it cannot wrap several pages the way it is rendered. So this bypasses having to change the renderNode method.

 initForm() {
    if (this._root.data.AcroForm) {                     <------------ Add this block
      // Form is already initialized
      return this;
    }
    if (!this._font) {
      throw new Error('Must set a font before calling initForm method');
    }
    this._acroform = {
      fonts: {},
      defaultFont: this._font.name
    };
    this._acroform.fonts[this._font.id] = this._font.ref();
   ...rest of method

Edit: Sorry, not sure how to format json...
Edit: Figured it out

@natterstefan
Copy link

Hi 👋 any update on this? Unfortunately I can't work on this myself at the moment, but maybe one of the community has the time and skills to do this. Thank you all!

@axel7083
Copy link
Author

Hi 👋 any update on this? Unfortunately I can't work on this myself at the moment, but maybe one of the community has the time and skills to do this. Thank you all!

Hey, sadly I do not have time to look at this PR, which is almost 2 years old. I will close it. Everyone is welcome to take the code I wrote.

@axel7083 axel7083 closed this Jul 11, 2024
@natterstefan natterstefan mentioned this pull request Aug 18, 2024
Open
3 tasks
@kjossendal
Copy link

I've got a good start on this but it's not really tested out thoroughly. It does what I needed though and could be expanded on. npm

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

@diegomura diegomura diegomura left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@axel7083 @jamesmfriedman @Ovyerus @mdefeche @majid701 @runelk @joneldiablo @kjossendal @natterstefan @diegomura