-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: master
Are you sure you want to change the base?
feat: added Form Annotation support #2845
Conversation
|
packages/primitives/src/index.js
Outdated
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'; |
There was a problem hiding this comment.
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)
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 |
@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
Hi @diegomura, 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
Option 2 - web standards
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)) |
Hey @diegomura, Hey @PhilippBloss, can you take a look at my last thoughts please? Thank you! |
**Reminder** Hey @diegomura, Hey @PhilippBloss, can you take a look at my last thoughts please? Thank you! |
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) |
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. |
Thanks @natterstefan ! Here are my thoughts:
|
Hi @diegomura, let's see what I can add to our discussion.
That is true, it's usefulness is "limited" (possibly an uninformed opinion) to creating a hierarchy of nodes as this can be achieved with 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?
Works for me.
They even share the same props as far as I am aware of. Sounds reasonable. What about Edit: After reconsidering it during the merge, I believe it’s better to keep the two separate ( |
Hey @diegomura, 👋 Have you seen my reply? I am hopeful that this will be merged asap. :) |
* upstream/master: chore(layout): bump yoga-layout fix: fixed HyphenationCallback type (diegomura#3019) chore: update READMEs to use ESM-first (diegomura#3032) chore: release packages (diegomura#3008) feat: init page box on layout fix: page size tests
This reverts commit 54fac40.
This reverts commit 3fe68cf.
Hi @diegomura, please take a look at my latest changes and edited answer thoughts here. //cc @PhilippBloss |
Tasks
Related PR
Notes
initForm
fix by @kjossendal, see feat: add Form Annotation support #2013 (comment)Docs
Example PDF
Example 2.0.pdf