dpinney / omf

The Open Modeling Framework for smart grid cost-benefit analysis.
https://omf.coop
GNU General Public License v2.0
112 stars 60 forks source link

Warn user if leaving gridEdit with unsaved changes #338

Closed dpinney closed 9 years ago

dpinney commented 9 years ago

Implementation will likely be:

window.addEventListener("beforeunload", function (e) {
    var confirmationMessage = 'It looks like you have been editing something.';
    confirmationMessage += 'If you leave before saving, your changes will be lost.';

    (e || window.event).returnValue = confirmationMessage; //Gecko + IE
    return confirmationMessage; //Gecko + Webkit, Safari, Chrome etc.
});
dpinney commented 9 years ago

Want to take a shot at this one?

mannanj commented 9 years ago

Yes sure

mannanj commented 9 years ago

grideditsavedchanges Got this going as desired with a tweak to each button click, so that it modifies a global variable to trigger the condition in window.unbeforeunload function:

//ADD PROMPT TO SAVE CHANGES BEFORE EXIT
window.onbeforeunload = function(e) {
    var confirmationMessage = 'It looks like you have been editing something. ';
    confirmationMessage += 'If you leave before saving, your changes will be lost.';    
    if (unsavedChanges) return confirmationMessage;
};

...

    <div class='buttonGroup leftToolbar'>
        <button class='pill' onclick='dropPill(this, "Pinning")'>Pinning ▾</button>
        <ul class='menu'>
        <li><a href='javascript:pinAll(); javascript:unsavedChanges=true' id='pinUnpinAll'>Pin All</a></li>
        <li><a href='javascript:unPinAll()'>Unpin All</a></li>
        <li><a href='javascript:toggleSelectedPin()'>Toggle Selected Pin (p)</a></li>
        </ul>
    </div>

So as an example, clicking "Pin All' (despite whether it changed from unpinned or was previously already pinned), brings up the prompt. Currently this code will have to be added to each button:

The code

javascript:unsavedChanges=true 

is what would have to be added to each button press.

Your thoughts?

dpinney commented 9 years ago

I like that approach. I was thinking about adding the unsavedChanges=true code to the javascript functions instead (e.g. inside pinAll()). But I like your method better because (1) its easier to figure out where to add it based on what's shown in the frontend and (2) it keeps the warning logic separate from the logic of each of the functions.

Note that you don't need the second javascript inside the href, it could just be:

<li><a href='javascript:pinAll(); unsavedChanges=true' id='pinUnpinAll'>Pin All</a></li>

Also, there's one button that's generated dynamically via javascript: the save button after user edits the properties of an object. It's generated in the functions at the bottom of gridEdit.js. The unsavedChanges=true global variable will have to be set there too.

mannanj commented 9 years ago

Thank you. I thought you would like it.

I had a problem with adding unsavedChanges to the javascript functions in gridEdit.js (like pinAll() or the save button). It seems the global won't save and pass to gridEdit.html. I'll take a crack at it again.

dpinney commented 9 years ago

Put unsavedChanges=true in a script tag in gridEdit.html. That will make it a global variable and allow you to change it as part of other functions (i.e. anything inside gridEdit.js).

mannanj commented 9 years ago

It is currently under the tag

<script type="text/javascript">
// GLOBAL VARIABLES:
var unsavedChanges

Which is where all the other globals also lie, and where the various functions are called. Am I missing something here?

mannanj commented 9 years ago

Nevermind, it was under a script tag in the html page, local to some of the functions in gridedit.html but not visible to those in gridEdit.js. Moved it out to top of page and it now works.

dpinney commented 9 years ago

Nice. Aren't closures fun? :neckbeard:

mannanj commented 9 years ago

Ofcourse :) Unfortunately, this one isn't closed yet. It still isn't appearing global. Seems it will take another crack at it.

mannanj commented 9 years ago

Encountered a "feature not a bug" when user saves a feeder. it takes you back to the feeders selection page (after pressing save). Seems to me to make more sense that user can continue working on the current feeder (add a Quit/Exit button to get out), unless 'Duplicate', 'Make Public', or 'Delete' is pressed in which case you do want to get out of the feeder.

dpinney commented 9 years ago

I think the issue with the global here is they you need to declare it before gridEdit.js is imported.

Encountered a "feature not a bug" when user saves a feeder...

Let's talk about this when we're both in the office. This behavior might be set up the way it is so the user experience with editing feeders in the gridlabMulti and solarEngineering models works well. We'll need to test some things. Otherwise, I agree with your suggested change (i.e. alert the user on save that the model was saved successfully but don't navigate away).

mannanj commented 9 years ago

So that global fix (creating global before gridEdit.js) didn't work and creates odd behavior. I don't think this is the way to do it. I read that gridEdit.js is actually loaded before any of the HTML code is run (making all global calls in the html page ineffective) so the variable must be made to be "on top of all windows", or glob.variableName should be used and neither type of solution worked.

Comment #3 says gridEdit.js is loaded before html page is: http://stackoverflow.com/questions/2932782/global-variables-in-javascript-across-multiple-files

Of course we could test it by moving the functions back to gridEdit.html but that would make it huge.

dpinney commented 9 years ago

I'm fine putting all the javascript back in gridEdit.html. It wouldn't make it that much longer. And gridEdit.js isn't imported anywhere else, so really the best place for it is back in gridEdit.html.

mannanj commented 9 years ago

Done!