show prompt only if form value changed

[BenGamma]

In how-to form, show the warning when leaving the page without saving only if a change as be made to the form. Should be doable using the FormState of react-final-form. Documentation here

[ivokh]

I started working on this issue so it can be assigned to me. Although I've been a software developer for a few years now I have no experience with React. I already ran into a few challenges where someone might have some ideas or suggestions for.

Dirty state It's pretty easy to only show the confirmation prompt when the react-final-form is dirty. The challenge is the logic behind the dirty state. React-final-form will use a shallow equal by default to detect changes between the initial value and the current value. This will not work for example for the tags because the tags value is an object. This can be solved by giving an isEqual callback to the field with some custom logic for the comparison. For file uploads it's not obvious what comparison logic to use. I suggest to set it to dirty/not equal when a new file is selected.

React Router Prompt The react router prompt component is used to display the confirmation alert. This only shows when the router is used to move to another page. The pp logo to go to the home page uses a href url and will not trigger the confirmation. The same applies for refreshing the page. I will have to look into the react router prompt to see if I can find a solution for this.

Testing edit page I'm currently testing on the create page. Because I have no access to edit a how-to. Should I create a test how-to to use the edit page or is there some way to give my user permission to edit other how-to's?

Automated tests I started looking onto the Cypress tests for this logic and I figured a way to test the confirmation alert. The question is how thoroughly we should test this logic? It's possible to validate the dirty stage of each field but that would result in a lot of tests. I think it might be a good idea to test one field with an integration/Cypress test and maybe validate the dirty state of the other fields with unit tests?

Adding the Lodash package For the comparison of the tags I added the Lodash package which has an isEqual function that does a deep comparison. Are there any problems with adding this package to the project?

[chrismclarke]

Thanks for taking the time to look at this and the depth of solution you've worked on here. I think the main issues you've addressed are more than enough, and we don't need to worry about going into 100% accuracy for things like images etc.

For the react router, another option could perhaps be seeing if we can use the componentWillUnmount lifecycle event for the form component instead (although I'm not sure if this will trigger on page reload or not, could be worth checking).

For the edit page testing, you can indeed just create any testing how-to that you want to use. At some point in the future I'm hoping to improve on this to run the platform fully locally, but this is still some ways away (time is not my friend unfortunately). For cypress, great that we have a test but again I'd say no point testing too extensively, if as you mentioned next we are doing form value comparison correctly then it shouldn't really make much difference.

And as you said for form comparison, in theory lodash shouldn't be much of an issue as it uses es modules and so is tree-shakeable (only adding in the chunk of code it needs so not bloating the overall site much at all). That being said I haven't tested the implementation myself, I've tried a couple specific npm packages like deep-equal and fast-deep-equal, and see there also react-fast-compare - I'm happy with anything that works (all seem to give different benchmarks of what is best and I don't expect any would be noticeably different for the small data sizes we are talking about for forms) although would be good to check is working as expected with a handful of form fields as I remember some have quirks depending on the order of keys the object is stored in etc.

In any of these cases I'm keen to get your changes in sooner and we can work out finer details later. I'll test a merge today and check everything is running as expected from this side and get back to you.

Thanks again for your input on this

[ivokh]

Thanks for the appreciation and the clear reaction. I did a quick check with the componentWillUnmount but it is not triggered when the page is refreshed so I think the best solution for now is in my pull request. Is there anything else I can look into to resolve/close this issue?