Remove revert button

[roll]

• fixes Revert button  #347
• fixes Discarding changes without warning when clicking on blank space (Left menu) #495

---

Please make sure that all the checks pass. Please add here any additional information regarding this pull request. It's highly recommended that you link this PR to an issue (please create one if it doesn't exist for this PR)

[cloudflare-workers-and-pages]

Deploying opendataeditor with    Cloudflare Pages

Latest commit:	a48e5b2
Status:	✅  Deploy successful!
Preview URL:	https://9210c40a.opendataeditor.pages.dev
Branch Preview URL:	https://347-revert-revert-button.opendataeditor.pages.dev

View logs

[guergana]

@roll Hi, why are these two changes together? they can't be separated?

[roll]

@guergana
No, there are not two changes. You can just remove the "Revert" button without the rest but to achieve "Unsaved Changes" dialog work on the application exit you need this logic consolidated (as previously it was per controller without a centralized way to handle unsaved changes)

[guergana]

@guergana
No, there are not two changes. You can just remove the "Revert" button without the rest but to achieve "Unsaved Changes" dialog work on the application exit you need this logic consolidated (as previously it was per controller without a centralized way to handle unsaved changes)

@roll Thanks for the clarification..there are many changes so it's a bit difficult to follow what's happening but I will take a closer look .

[roll]

@guergana
Thanks! Yeah I spent quite a lot of time figuring out the working logic for reacting on the app closing event and nothing worked within the separated controllers architecture

[guergana]

@roll the revert button is succesfully removed but I tried changing a field and opening a different file and I didn't get the popup saying there are unsaved changes.. is this still a requirement for this ticket?

I tried some more times and I see the pop up does show up but inconsistently. like if you edit a field and immediately change to another file, sometimes you can see the popup flashes but disappears, and sometimes it doesnt show up at all :/

[romicolman]

Hi! I tested this on Mac.

Scenario 1: Quit the ODE DISCARDING changes

• I opened the ODE.
• Selected a file on the left menu.
• Made a change on a specific cell and then clicked Quit ODE on the top menu:

Then, I clicked DISCARD and got this message:

Scenario 2: Quit the ODE SAVING changes

• I opened the ODE.
• Selected a file on the left menu.
• Made a change on a specific cell and then clicked Quit ODE on the top menu:

Then, I clicked SAVE and got this message:

We need to remove this second dialog in both scenarios. It adds an unnecessary extra action to the user.

@guergana I reproduced what you pointed out and I agree. There is something going on there..

• I opened the ODE.
• Made a change to a cell on a specific file and did not click SAVE to update changes.
• Selected a different file on the ODE left menu and never got the dialog to save changes.

I'm creating a new ticket for this

[roll]

@guergana
Can you please test it again? I fixed the in-app changes problem

[roll]

We need to remove this second dialog in both scenarios. It adds an unnecessary extra action to the user.

It's easy to remove but note that without a second dialog, it looks quite weird for the user -- e.g. if you click on "Save" something happens and loads and then suddenly apps close. But of course, I can remove it if needed

[romicolman]

Just in case, we are talking about removing the second dialog, which appears after the user goes to the top menu and selects "Quit the ODE".

We are applying the same logic that Excel has. The only difference is that Microsoft offers a third option: Cancel. Let's talk about this in the planning meeting.

[roll]

@romicolman
@Faithkenny
For now, I'll add a progress indication to the dialog and when it's done will close the app without second dialog

[romicolman]

Perfect! Thanks!

[roll]

@romicolman
I tried different options with progress indicators, but I'm not sure what is the best for UX as they are to fast to be perceived on small fiels, so for now, I just removed the second dialog. Can you please try?

[romicolman]

Hi @roll! I tested this implementation.

The Revert button is no longer on the datagrid, so that's OK.

In terms of leaving the app without saving changes:

• I opened the ODE.
• Clicked on a file in the left menu.
• Edited one of the cells without saving changes.
• Then went to Open Data Editor----Quit..

The application closed without showing the dialog to confirm or discard changes:

Grabacion.de.pantalla.2024-08-19.a.la.s.10.12.49.a.m.mov

However, the dialog is display after you click on a blank space in the datagrid:

Grabacion.de.pantalla.2024-08-19.a.la.s.10.19.09.a.m.mov

[roll]

Hi @romicolman,

Thanks! Note the as the not committed edits is a different issues it's no included into this PR. You can test altogether in #517

[romicolman]

Perfect!! Thanks!!

[roll]

@romicolman
Sorry I was wrong -- the in-editing fix required another PR to be merged first.

I think it would make sense to test and merge this one first. It should work for any unsaved changes leaving situation except for ones when you have an active cell editing. And this one will be fixed by #517

[romicolman]

@roll I tested this issue. It works when..

• The user edits a cell.
• Clicks outside the table without clicking SAVE or DISCARD.
• and selects Quit ODE in the Open Data Editor top menu.

Grabacion.de.pantalla.2024-08-21.a.la.s.9.31.01.a.m.mov

But...if you edit the cell and do not click outside of the table (SAVE and DISCARD buttons do not get activated) the tool closes unexpectedly. I think the solution is to add a third button: CANCEL

Grabacion.de.pantalla.2024-08-21.a.la.s.9.47.09.a.m.mov

Also, when you edit a cell and click on the blank space, select Quit ODE, but do not choose any of the options available, the tool closes unexpectedly:

Grabacion.de.pantalla.2024-08-21.a.la.s.9.47.09.a.m.mov

[roll]

Hi @romicolman

I can't reproduce it:

I think it MacOS specific problem where the outside click is not detected. It's easy to fix if we handle table editing state on the quick click (cc @guergana).

I recommend merging this PR (as generally it fixes the core problem esp #347) and creating a follow-up issue regarding MacOS problem otherwise we will get in too many merge conflicts when I'm back in 3 weeks.

PS.
I fixed the case when you click outside the closing dialog and the app closes anyway. Now the uses just have to decide between "Discard" and "Save". If "Cancel" is needed, similarly it will be better to handle it in a separate issue

[pdelboca]

Thanks @roll ! I'm merging this so we can work on any issue that might happen when you are away.