pdfme / pdfme

A TypeScript based PDF generator library, made with React.
https://pdfme.com
MIT License
2.19k stars 194 forks source link

Fix to allow zero values for form fields #367

Closed peteward closed 5 months ago

peteward commented 5 months ago

Fixes #366

I can't see any negative impact of this change.

It's still technically possible to break a schema by forcibly deleting a required value and ignoring the red form error message, but it's a lot better than having the value removed from the schema entirely if it's value is 0.

You can still remove values such as rotation from the schema, but by deleting the values not by setting them as 0. If we wanted to do this we would need to check if these fields are optional or not then remove them. This is more complex to implement.

https://github.com/pdfme/pdfme/assets/7068515/b3350f0c-111d-48ec-8c1a-5a100d4ee4ba

vercel[bot] commented 5 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **pdfme-playground** | ⬜️ Ignored ([Inspect](https://vercel.com/labelmake/pdfme-playground/42scuWt2jnufCNz1AJ5TibAUqRCJ)) | [Visit Preview](https://pdfme-playground-git-pw-allow-zero-values-labelmake.vercel.app) | | Dec 16, 2023 9:39am |
hand-dot commented 5 months ago

Hey @peteward , I checked. I added this code a few weeks ago to solve this problem. https://github.com/pdfme/pdfme/blob/main/packages/ui/src/components/Designer/Sidebar/DetailView/index.tsx#L80

However, with this PR, the problem seems to have reoccurred.

https://github.com/pdfme/pdfme/assets/24843808/decefe70-249a-4578-9e3a-b7d988b0637b

https://github.com/pdfme/pdfme/issues/366#issuecomment-1856185506 You are right. My last bug fix is not perfect.

While it's not the most elegant solution, hardcoding to assign undefined to the opacity and rotate values if they are null could be a feasible workaround. What do you think?

peteward commented 5 months ago

Ok, I've updated as you have suggested.

Longer-term this needs some thought. Ideally we should be able to use a definition to check and enforce an appropriate value. I tried pulling in the Zod definition but it was getting messy, and it only describes the core shared attributes of schema, not the additional attributes extended by each schema. I also tried getting the propPanel from the schema and trying to check if the field was required or not, but that wasn't easy and doesn't necessarily align to the problem we have here.

Also, it's still possible to get into a bad state because there's no enforcement of values:

https://github.com/pdfme/pdfme/assets/7068515/1bcc9a14-cd71-4aee-9c7b-8ee88162af4c

I think we do need to make the validation state of the template more easily available. At least that would help with the above issue.

hand-dot commented 5 months ago

Please ignore failing test. also i create new issue about often fail CI. https://github.com/pdfme/pdfme/issues/370

peteward commented 5 months ago

Good teamwork!!

hand-dot commented 5 months ago

I know! we are good team👍