form-js icon indicating copy to clipboard operation
form-js copied to clipboard

Display correct submit data in playground output panel

Open pinussilvestrus opened this issue 2 years ago • 2 comments

Closes #273

As discussed, we shall always display the submit data in the output panel. We simply do a form.submit to rely on the exact same data, instead of using the current form state (as we do beforehand).

Demo to try it out: https://demo-submit-panel--camunda-form-playground.netlify.app/

Kapture 2022-10-18 at 13 34 42

pinussilvestrus avatar Oct 18 '22 11:10 pinussilvestrus

@vsgoulart adding you as a reviewer as well. Not expecting you to do a proper review before getting the form-js introduction, but maybe interesting already 🙂

pinussilvestrus avatar Oct 18 '22 11:10 pinussilvestrus

I noticed some warnings about the dependency array for the useEffectss Generally is not a good idea to skip the dependencies in Reacts useEffect dependency array (I'm assuming it's the same for Preact)

What's the strategy for this? And this is a conscious decision shouldn't we just skip the linting for such cases?

Once we added the linting rules, I think it was a decision to not strictly follow the missing dependencies recommendation but to show them as warnings, which is the default (cf. https://github.com/bpmn-io/form-js/pull/171).

There are quite a few occurrences (npm run lint) of that in the whole package, not only in form-js-playground. We always had the rule of thumb to not over-optimize for performance at the beginning but only when we see the necessity. So far, there was no need for it, so we kept most effects as they are; the same goes for this one.

pinussilvestrus avatar Oct 18 '22 12:10 pinussilvestrus

We always had the rule of thumb to not over-optimize for performance at the beginning but only when we see the necessity. So far, there was no need for it, so we kept most effects as they are; the same goes for this one.

The biggest issue with not providing filling the deps array is that it could create bugs in special cases, the link I posted explains it a bit. But anyway this is not the main topic of this PR and we can discuss it later as it evolves as a team. Regarding the warnings, I'm the opinion that they should be treated as bugs and try to remove them ASAP when they appear. If we're just ignoring them they're just noise which will make us more likely to ignore it when something more serious happen. (But this also is a topic for another time)

I also noticed that you pasted the env URL manually. Do we have any sort of automations to deploy branch envs? If not, we could do something like what the Console team is doing here with Cloudflare pages.

vsgoulart avatar Oct 19 '22 15:10 vsgoulart

@vsgoulart all valid topics you bring on, let's discuss them in an orderly round (e.g. weekly additional topics 👍 )

I also noticed that you pasted the env URL manually. Do we have any sort of automations to deploy branch envs? If not, we could do something like what the Console team is doing here with Cloudflare pages.

The demo is just something I created personally for the initial Camunda playground spike, and I (ab)used it for this branch as well. I really loved to see something automatic. Netlify also provides automatic branch deploy so I could easily set it up. I will add an agenda point to the next weekly 👍

pinussilvestrus avatar Oct 19 '22 19:10 pinussilvestrus

Open for review again.

pinussilvestrus avatar Oct 20 '22 09:10 pinussilvestrus