New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display a modal when saving a user script in the editor #2858

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Sxderp
Contributor

Sxderp commented Feb 17, 2018

fixes #2847

@arantius arantius added this to the 4.3 milestone Feb 18, 2018

@arantius

This comment has been minimized.

Collaborator

arantius commented Feb 18, 2018

FYI it's very helpful for me when you put Fixes #NNN in the commit message, not (only) the PR description. Then GitHub automatically links things at merge time.

@@ -290,6 +290,14 @@ async function saveUserScript(userScript) {
onSaveError(e.target.error);
reject(e);
db.close();
} finally {
// Send the script change event, even though the save may have failed.

This comment has been minimized.

@arantius

arantius Feb 18, 2018

Collaborator

This worries me. If the save failed, I don't want the editor changing, and maybe throwing away unsaved details.

This comment has been minimized.

@Sxderp

Sxderp Feb 18, 2018

Contributor

The userScript that's being saved should represent the editor as it is. You're basically returning the same details you submitted to the background, along with any downloaded resources. You shouldn't lose any unsaved details as userScript represents all those unsaved details (that failed to save).

This comment has been minimized.

@Sxderp

Sxderp Feb 18, 2018

Contributor

Hm, I suppose that if the external resources take a long time to download time you may be able to perform some edits before the async save is returned. But that issue applies to both successful saves and unsuccessful saves.

I think the best thing to do is popup some modal that says 'saving' and doesn't go away until the result is back. If a save error occurs then display the error in the modal. If no errors, then auto close.

Sxderp added some commits Feb 18, 2018

Resolve script details on save
By resolving the details directly there is no need for the
"UserScriptChange" message to be sent to the editor.
Remove onMessage from user script editor
No longer necessary to listen for "UserScriptChanged". All results will
be passed directly into the response of the initial save message.
Add a saving modal for the editor
When saving a popup modal indicating a save is in progress is displayed.
The modal also prevents any modifications and subsequent data lose
during the asynchronous save.  Once the save is complete close the
modal. On Error display the message in the modal.

@Sxderp Sxderp changed the title from Send the update message when saving a user script to Display a modal when saving a user script in the editor Feb 18, 2018

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Feb 18, 2018

Updated with a different approach.

@arantius

This comment has been minimized.

Collaborator

arantius commented Feb 21, 2018

I see in an email update (but can't find anywhere else) that @Sxderp said:

I think the best thing to do is popup some modal that says 'saving' and doesn't go away until the result is back. If a save error occurs then display the error in the modal. If no errors, then auto close.

I agree.

@arantius

This comment has been minimized.

Collaborator

arantius commented Feb 21, 2018

I plan on merging this, but after some other work of mine -- I'll have to resolve merge conflicts at that time.

While we're waiting: does CodeMirror not have something to do (possibly modal) dialogs already? Maybe https://github.com/codemirror/CodeMirror/tree/master/addon/dialog ?

If not is there a tidy general JS "modal dialog" component somewhere that we could simply reuse, rather than writing support for it from scratch? A few moments of searching finds me:

Which seem decent. The smaller/lighter the better IMO. Anything pre-existing with a documented API is good.

@Eselce

This comment has been minimized.

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Feb 21, 2018

If you want simple, I don't think you could get simpler than the code I wrote. It just uses rivets for the content and displays / undisplays by changing the body class (like the Monkey Menu).

Is it necessary for a "full feature" modal of some sort?

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Feb 22, 2018

When I have time I'll work on this for the recent changes to master. Should be done by Monday.

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Feb 24, 2018

I'm going to wait on this until we have better error handling in the downloader.

@arantius arantius modified the milestones: 4.3, 4.4 Feb 28, 2018

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Mar 10, 2018

Closing this and reopening as a new branch / PR

@Sxderp Sxderp closed this Mar 10, 2018

@Sxderp Sxderp referenced this pull request Mar 10, 2018

Merged

Use save modal with editor #2900

@arantius arantius removed this from the 4.4 milestone Mar 23, 2018

@Sxderp Sxderp deleted the Sxderp:send-script-change-message-to-editor branch Mar 31, 2018

arantius added a commit to arantius/greasemonkey that referenced this pull request Jun 7, 2018

@arantius arantius added this to the 4.5 milestone Jun 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment