react-pdf icon indicating copy to clipboard operation
react-pdf copied to clipboard

feat: added Form Annotation support

Open natterstefan opened this issue 1 year ago • 4 comments

Tasks

  • [x] fix dependency cycle (also mentioned here). @diegomura: I am not sure how though... #need-help
  • [ ] discuss component names (see https://github.com/diegomura/react-pdf/pull/2845#discussion_r1721040015)
  • [x] fix macOS (https://github.com/diegomura/react-pdf/pull/2845#discussion_r1734780553)

Notes

Docs

Example PDF

Example 2.0.pdf

natterstefan avatar Aug 18 '24 19:08 natterstefan

⚠️ No Changeset found

Latest commit: 2493b996dec8e7e199bd381fe685112e8ec0866e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Aug 18 '24 19:08 changeset-bot[bot]

Right now, push buttons don't have any purpose. They are only there to exist. They cannot hold any value. There should be an option for the user to specify an 8.6.4 action (from PDF Reference 1.7) for this button. Even if it's just on a basic level. Going too detailed e.g. with 8.5.3 actions would go beyond the scope of the pr. What do you think?

PhilippBloss avatar Aug 25 '24 17:08 PhilippBloss

Right now, push buttons don't have any purpose. They are only there to exist. They cannot hold any value. There should be an option for the user to specify an 8.6.4 action (from PDF Reference 1.7) for this button. Even if it's just on a basic level. Going too detailed e.g. with 8.5.3 actions would go beyond the scope of the pr. What do you think?

Yes, it's beyond this scope. Let's keep it "small" by introducing the first version of form elements first and then improve from there.

natterstefan avatar Aug 28 '24 14:08 natterstefan

Hi @diegomura, what do you think about this feature request?

natterstefan avatar Sep 06 '24 13:09 natterstefan

Would love to see this get merged! I'd love to contribute as well if there's a blocker

GauravB159 avatar Nov 14 '24 07:11 GauravB159

@PhilippBloss thanks for the review here. Would love to take this to the finish line. Are you still open to take care of the docs? 🙏🏻

diegomura avatar Nov 17 '24 01:11 diegomura

@PhilippBloss thanks for the review here. Would love to take this to the finish line. Are you still open to take care of the docs? 🙏🏻

Initially I wanted to wait for a decision on the component names, but I made a todo: https://github.com/diegomura/react-pdf-site/pull/145

PhilippBloss avatar Nov 19 '24 15:11 PhilippBloss

Hi @diegomura, Hi @PhilippBloss,

I just made the branch compatible with 4.x and took care of @PhilippBloss' feedback. The only that is left now is deciding which names we choose for the components:

Option 1 - align with pdfkit

  • <FormField />
  • <FormText />
  • <FormCombo />
  • <FormCheckbox />
  • <FormList />

Option 2 - web standards

  • <Fieldset />
  • <InputText /> (not 100% aligned as it would be <input type="text" />)
  • <Select />
  • <Checkbox /> (Alternative InputCheckbox, anyway not 100% aligned as it would be <input type="checkbox" />)
  • <List /> (also not 100% aligned but there are ol and ul lists)

I favour Option 1 more as there is a connect to the used library used for forms. What do you think?

(previous discussion: https://github.com/diegomura/react-pdf/pull/2845#discussion_r1721040015, and https://github.com/diegomura/react-pdf/pull/2845#discussion_r1783965707)

natterstefan avatar Dec 06 '24 09:12 natterstefan

Hey @diegomura, Hey @PhilippBloss,

can you take a look at my last thoughts please? Thank you!

natterstefan avatar Dec 10 '24 07:12 natterstefan

**Reminder**

Hey @diegomura, Hey @PhilippBloss,

can you take a look at my last thoughts please? Thank you!

natterstefan avatar Jan 04 '25 22:01 natterstefan

Hi @natterstefan

Sorry for my slow response here. I took a long vacation on the last couple of weeks.

Thanks for listing both options. In my opinion something like option 2 is what we need. Being coherent with pdfkit is not really important, as pdfkit is an implementation detail and not something users are really aware of.

React-pdf always followed react-native primitives, so if an element is already defined there we should use the same naming (ex. TextInput instead of InputText or FormText)

diegomura avatar Jan 05 '25 19:01 diegomura

Hi @diegomura,

no worries. I hope you had a wonderful time. :)

This seems reasonable and let's follow that approach here as well.

It works for text input, but what about other components of this PR?

Checkbox is not part of react native anymore: https://reactnative.dev/docs/checkbox, also forms in general.

natterstefan avatar Jan 06 '25 12:01 natterstefan

Thanks @natterstefan !

Here are my thoughts:

  • FormField: Why do we need this one? It's not semantic nor clear what this does for the user. Pdfkit docs says that to express hierarchy a name can be passed to each form element
  • TextInput: Like the react-native primitive
  • Select: not in primitives but good name
  • Checkbox: not in primitives but good name
  • List: Can be merged with Select maybe, and add a type="list" prop? I feel they are basically the same element, it's just rendered differently

diegomura avatar Jan 11 '25 03:01 diegomura

Hi @diegomura,

let's see what I can add to our discussion.

  • FormField: Why do we need this one? It's not semantic nor clear what this does for the user. Pdfkit docs says that to express hierarchy a name can be passed to each form element

That is true, it's usefulness is "limited" (possibly an uninformed opinion) to creating a hierarchy of nodes as this can be achieved with name only as well. Setting defaults (such as the default font) for all child form annotations (docs) is another feature. I understand that you question its purpose in the react-pdf context. Maybe if we can answer this you can make the final decision: Does FormField help creating a cleaner, more hierarchical PDF (e.g. using parent of pdfkit)? If one prefers another component layer instead of using name only then yes. Does it help to simplify shared styling/attributes? Yes, but one can achieve this also by sharing props or styles (correct?). ~~So what about removing it and if people request it add it (again) in another release?~~

Edit: but if we remove it, do we need to keep https://github.com/traveltechdeluxe/react-pdf/blob/0b6f34ed42101aedbefe6c5dd74d6e542ebca51b/packages/pdfkit/src/mixins/acroform.js#L105-L117? Edit 2: I would keep them for now and consider deprecating them later if feedback suggests they add no value.

  • TextInput: Like the react-native primitive
  • Select: not in primitives but good name
  • Checkbox: not in primitives but good name

Works for me.

  • List: Can be merged with Select maybe, and add a type="list" prop? I feel they are basically the same element, it's just rendered differently

They even share the same props as far as I am aware of. Sounds reasonable. What about type="combo|list then as possible type values (in sync with pdfkit)?

Edit: After reconsidering it during the merge, I believe it’s better to keep the two separate (List and Select). This approach not only simplifies the overall structure but also reduces the number of branches in the code, making it easier to maintain and debug. By maintaining clear separation, it also improves readability and ensures that each function (aka Component) has a distinct responsibility, although they look similar.

natterstefan avatar Jan 12 '25 20:01 natterstefan

Hey @diegomura, đź‘‹

Have you seen my reply? I am hopeful that this will be merged asap. :)

natterstefan avatar Jan 14 '25 16:01 natterstefan

Hi @diegomura, please take a look at my latest changes and edited answer thoughts here. //cc @PhilippBloss

natterstefan avatar Jan 19 '25 22:01 natterstefan

would be great to get this reviewed and merged if possible!

chrisdevereux avatar Jan 22 '25 11:01 chrisdevereux

would be great to get this reviewed and merged if possible!

Hi, I agree with you and I hope we can merge it soon. We need it as well urgently in one of our projects.

@diegomura please, take a look and let's get this done! âś… thank you

natterstefan avatar Jan 22 '25 12:01 natterstefan

Hey @diegomura. I would be happy if you could give feedback in the next few days. I think some people are waiting for this a well. We also had to use a temporary workaround to be able to use it for our customers already. Please let me know where and how I can help you. Thank you and have a nice day.

natterstefan avatar Jan 28 '25 07:01 natterstefan

Hey, I would love to see this getting merged. @natterstefan Is there any way, that you could expand the scope of this pull request a bit and add the possibility to embed signature fields? If I understand it correctly pdfkit needs to be extended here. According to the Acrobat Javascript API reference the field type would be signature. Would love to hear your opinion on this. Thanks in advance!

jub-nick avatar Jan 28 '25 11:01 jub-nick

Hi @jub-nick,

Thanks for your interest! I appreciate the suggestion to expand the scope of this pull request to include signature fields. It sounds interesting and I am sure it would add even more value to react-pdf.

However, at the moment, I suggest adding another PR for your request to get this PR live asap. I am also not 100% sure if I added all supported props of the signature field. Having said that here is a draft PR that is ready once this one is merged: https://github.com/traveltechdeluxe/react-pdf/pull/3.

Additionally I added another one for radiobutton but that one is not working yet, especially on macOS: https://github.com/traveltechdeluxe/react-pdf/pull/4. I am also more than happy to prepare it further once this PR is merged.

Looking forward to any contributions—thanks again for your input!

natterstefan avatar Jan 29 '25 21:01 natterstefan