Added opacity option to all schemes and fix rotation bugs

peteward commented 6 months ago

Ref: https://github.com/pdfme/pdfme/issues/336

Opened on behalf of @LeandroBanaszeski

vercel[bot] commented 6 months ago

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

1 Ignored Deployment

hand-dot commented 6 months ago

@peteward I'd like to hear your opinion on this feature.

I think an input UI for a number range from 0 to 1 would be better since the slider UI seems a bit exaggerated. Also, similar to rotate, I believe we need controls such as disabling it when opacity is not specified in the plugin's propPanel.defaultSchema.

If you agree with the above, I can continue with the implementation on my end.

Personally, I don't think it's an absolutely necessary feature, but if it doesn't interfere with the designer's UI, I do think it's a nice feature to have.

peteward commented 6 months ago

@hand-dot ,

Whilst not a high priority feature, it clearly has a use-case for @LeandroBanaszeski and it seems quite simple for both the UI and pdf-lib so I think we should add it.

I think the slider is OK, but not at full width, it takes up too much space. It also seems a to be visually clipped at the edges, and buggy when I put it at the end of a row - it causes the UI to refresh for me. So, perhaps just keep it simple with a number input.

Agree that we should make it optional 👍

peteward commented 6 months ago

If the items on the sidebar get much higher, we might need to consider making the sidebar wider so that we can fit 4 number widgets on one row (and/or adjust the CSS to reduce horizontal padding slightly).

But one more row for now should be OK...

hand-dot commented 6 months ago

@peteward Thank you for your opinion. I will ask for a review once the implementation is done.

Thank you always

hand-dot commented 6 months ago

Hey @peteward, Could you pls review ?

Opacity Works like this.

https://github.com/pdfme/pdfme/assets/24843808/3884471d-439c-4c94-97f5-8a2b285c8ecc

CleanShot 2023-11-28 at 10 58 02@2x

Also fix rotate degrade: https://github.com/pdfme/pdfme/pull/337

https://github.com/pdfme/pdfme/assets/24843808/7a6bec3d-bfa3-4951-be49-4a1cd4219494

hand-dot commented 6 months ago

Hey @peteward

What was the fix for the rotation issue?

The most recent commit should have fixed both issues.

https://github.com/pdfme/pdfme/assets/24843808/fdda8c3f-a16b-47a3-ad35-3dcd511cd773

I uploaded a version of the schema after removing the optional rotation field to trigger the bug.

Where should i check?

I apologize for ending up combining both the opacity feature and rotate bug fix into one PR, even though you had separated them into different PRs...

peteward commented 6 months ago

no need to apologise, great work! I am sorry for breaking it :D

I can confirm this fix with the local template I made

peteward commented 6 months ago

Hi @hand-dot,

This appears to have introduced a new issue:

https://github.com/pdfme/pdfme/assets/7068515/af2808fe-2ced-4847-8a5d-d42512d33145

Select a schema
Enter a value into one of the inputs
Select a different input
The schemas now keep selecting and unselecting when you click one of them.

hand-dot commented 6 months ago

@peteward

Oh....., okay i'll check it! thank you!

hand-dot commented 6 months ago

@peteward Reproducing the issue is not possible, is there something wrong with the procedure?

https://github.com/pdfme/pdfme/assets/24843808/004d7e51-a20f-4674-8900-546b1131608f

peteward commented 6 months ago

Apologies for the slow reply. I will try a fresh checkout on this later today and confirm.

peteward commented 6 months ago

I've done a fresh checkout, switched to this branch and built as before. I'm seeing the same bouncing behaviour when I edit more than one input on a schema.

peteward commented 6 months ago

Ok, I have figured this out with @steffancarrington's help!

When you resize the screen, or the container size changes, the sidebar re-renders to the list view, even if you are on the detail view.
When you increase opacity > 1 you get the red error message displayed.
In my version of Chrome, this error message caused the container size to grow and create a scrollbar around the sidebar. This triggered a re-render, which closed the sidebar and started a bounce loop.
When I updated to the latest Chrome this problem did not happen.

I think there is a bug here that container or screen size changes that trigger re-render should not lose the selected schema state, but it is not related to this PR.

I will log this separately to look at.

peteward commented 6 months ago

hmm on second thoughts, I think it is something in this PR.

I will try again to find it tomorrow.

hand-dot commented 6 months ago

Hey, @peteward

Thank you for taking the time to debug. I respect your thorough quality check.

OK, I'll still hold off on merging this. However, I don't want the PR to become outdated and neglected, so please feel free to let me know if there's anything. I will also do some debugging on my end.

peteward commented 6 months ago

this is very strange...

I think it is related to a LastPass browser extension that I have.

What is strange is that just adding the opacity field definition in creates the problem and seems to activate LastPass on the form. If I comment this out, then I do not see LastPass running on these fields:

      opacity: {
        title: i18n('opacity'),
        type: 'number',
        widget: 'inputNumber',
        disabled: defaultSchema?.opacity === undefined,
        dataLpignore: true,
        max: 1,
        min: 0,
        span: 8,
      },

->

Screenshot 2023-12-02 at 16 26 48

I don't know what it is about this field that seems to trigger Lastpass.

I will try to look at what is losing the schema selection when the screen resizes, I think this will resolve the issue.

peteward commented 6 months ago

@hand-dot, the issue I was seeing was due to Lastpass triggering a resize of the component which caused the <React.StrictMode> issue to rerender the screen (#347)

This PR should be good to merge :+1:

hand-dot commented 6 months ago

@peteward Thank you!

hand-dot commented 6 months ago

@LeandroBanaszeski Thank you for your contribution!

pdfme / pdfme

Added opacity option to all schemes and fix rotation bugs #338