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

feat: added Form Annotation support #2845

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

natterstefan
Copy link

@natterstefan natterstefan commented Aug 18, 2024

Tasks

Related PR

Notes

Docs

Example PDF

Example 2.0.pdf

Copy link

changeset-bot bot commented Aug 18, 2024

⚠️ No Changeset found

Latest commit: b6995a3

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

Comment on lines 11 to 16
export const Form = 'FORM';
export const FormField = 'FORM_FIELD';
export const TextInput = 'TEXT_INPUT';
export const FormPushButton = 'FORM_PUSH_BUTTON';
export const Picker = 'PICKER';
export const FormList = 'FORM_LIST';
Copy link
Author

Choose a reason for hiding this comment

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

What do you think of keeping the initial names of these components to keep them aligned with the form annotation methods of PDFKit?

formText( name, x, y, width, height, options)
formPushButton( name, x, y, width, height, name, options)
formCombo( name, x, y, width, height, options)
formList( name, x, y, width, height, options)

(src)

@PhilippBloss
Copy link

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?

@natterstefan
Copy link
Author

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

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.

I love this. Thanks so much for putting this together. I'd like to get this merged soon.

Left a small comment about primitive naming.

Also I think we should add docs in the site (https://github.com/diegomura/react-pdf-site) after this lands. Can you take care of them? 🙏🏻 Not a blocker to merge this once we settle on namings

@@ -8,6 +8,11 @@ export const Note = 'NOTE';
export const Path = 'PATH';
export const Rect = 'RECT';
export const Line = 'LINE';
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.

Not a strong opinion, but would Form be more semantic? For Field I imagine like an input, but this is actually a form wrapper

Copy link
Author

Choose a reason for hiding this comment

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

Hi @diegomura,

thanks for your response. Let's see what I suggested here in the past: #2845 (comment). 🤔

My initial thought was to align the names with the ones used in pdfkit (here). So TextInput becomes FormText, Picker becomes FormPicker, and so on. But you made a valid point back in 2022 suggesting we should stick to the native web primitives. I got used to the current names while preparing the PR.

Regarding FormField: I chose the same name as pdfkit mainly to align our (react-pdf) and their (pdfkit) docs and keep them similar. I don't know if that's practical.

Choose a reason for hiding this comment

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

What about something like FormGroup? Form rather sounds like the single and complete set of inputs, but they rather are a subsection. E.g., you could separate billing address and shipping address into two groups.

Copy link
Author

@natterstefan natterstefan Oct 2, 2024

Choose a reason for hiding this comment

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

Hi @PhilippBloss,

your suggestion makes sense to me, considering what pdfkit states in their docs as well:

Using the formField method you might create a shipping field that is added to the root of the document, an address field that refers to the shipping field as it's parent, and a street Form Annotation that would refer to the address field as it's parent.

What do you think of fieldset(https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset):

The <fieldset> HTML element is used to group several controls as well as labels (<label>) within a web form.

Choose a reason for hiding this comment

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

Sounds good as well. Would give the decision to @diegomura

Choose a reason for hiding this comment

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

Would take over the docs, except @natterstefan has already started with those

Copy link
Author

Choose a reason for hiding this comment

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

Would take over the docs, except @natterstefan has already started with those

Hi @PhilippBloss, I haven't taken care of the docs yet. You can take over the docs if you want to or we can share the task. It's fine for me.

Also thanks for the review. I will try to apply your suggestions tomorrow or at least in the next few days.

Thanks for the ping @diegomura.

@GauravB159
Copy link

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

@diegomura
Copy link
Owner

@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? 🙏🏻

PhilippBloss added a commit to PhilippBloss/react-pdf-site that referenced this pull request Nov 19, 2024
@PhilippBloss
Copy link

@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: diegomura/react-pdf-site#145

# By Diego Muracciole (31) and others
# Via GitHub
* upstream/master: (60 commits)
  feat: allow units for page size (diegomura#2773)
  chore: bump jay-peg
  chore: release packages (diegomura#2981)
  fix(textkit): make indentation only affect first line. (diegomura#2975)
  fix: conditional rendering (diegomura#2983)
  fix(reconciler): missing dependencies (diegomura#2980)
  chore: release packages (diegomura#2959)
  feat: rem border widths (diegomura#2960)
  fix: add scheduler dependency (diegomura#2958)
  chore: update README
  chore: release packages (diegomura#2953)
  feat: support rem units (diegomura#2955)
  feat: add image stress test example (diegomura#2954)
  feat: support multiple line-height units (diegomura#2952)
  chore: release packages (diegomura#2946)
  fix: note rendering (diegomura#2951)
  feat: accept commas between transformations (diegomura#2950)
  fix: document metadata setters (diegomura#2949)
  fix: stroke dash array computation (diegomura#2948)
  feat: remove cross-fetch (diegomura#2947)
  ...

# Conflicts:
#	packages/renderer/index.d.ts
@natterstefan
Copy link
Author

natterstefan commented Dec 6, 2024

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: #2845 (comment), and #2845 (comment))

@natterstefan
Copy link
Author

Hey @diegomura, Hey @PhilippBloss,

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

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.

6 participants