form-js
form-js copied to clipboard
Display correct submit data in playground output panel
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/
@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 🙂
I noticed some warnings about the dependency array for the
useEffects
s Generally is not a good idea to skip the dependencies in ReactsuseEffect
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.
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 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 👍
Open for review again.