Use save modal with editor#2900
Conversation
|
With this, every single save action causes the entire window to blink/flash, even if it completes "instantly". I don't like that. This is a strange behavior for a text editor. (If I haven't added remotes, it should "just save" with no dialog. If I have, then and only then it makes sense to show the progress of those downloads.) |
|
While I can see your point and I have some ideas on how to implement those checks (may require reworking of I suppose we could mark it clean before the save and mark it dirty if there's an error. But that might also seem like bad UX. |
|
A "normal" editor I think always assumes that a filesystem write is always fast. It doesn't have to plan to remotely download things over the internet as part of a save action. I can't think of another text editor that ever has any kind of progress/dialog as part of a save action, that I've used. (Also, neither VM nor TM do this.) I think the simplest (best?) solution would be to delay showing the modal for some amount (100ms?) of time. Enough that we expect that a plain save (no new remotes to download) will always have passed. If that has passed and we're still saving, then show the dialog with its progress bar. If not, just never show the dialog. Either way, mark clean only when save has finished. It's not too complicated on top of that to track clean/dirty of parallel save and edit actions. Just a few cases to design in. |
I've seen editors that save synchronously. Most of the time the editor just freezes rather than bring up a dialog box. |
dd41257 to
e2d279d
Compare
|
Updated with a simple timeout before showing the save modal. Much easier than anything I had originally thought of (which involved doing checks to see if we actually needed to fetch remotes, etc.). |
src/content/edit-user-script.css
Outdated
|
|
||
|
|
||
| body.save #modal | ||
| { display: grid; } |
There was a problem hiding this comment.
Rarely all on one line, otherwise always:
selector {
rules
}
| height: 100% !important; | ||
| } | ||
| .CodeMirror pre { | ||
| line-height: 14px !important; |
There was a problem hiding this comment.
Why? Does this have anything to do with showing a modal dialog? Some side effect of now including shoelace?
There was a problem hiding this comment.
Slide effect of shoelace.
src/content/edit-user-script.html
Outdated
| </ul> | ||
| </div> | ||
| </div> | ||
| <div class="panel-foot"> |
src/content/edit-user-script.html
Outdated
| <body> | ||
| <div id="modal"> | ||
| <div class="panel"> | ||
| <div class="panel-head"> |
|
|
||
| <body> | ||
| <div id="modal"> | ||
| <div class="panel"> |
There was a problem hiding this comment.
Why do I need an extra wrapper div here?
There was a problem hiding this comment.
The outer div covers the entire page and consists of a semi transparent dark color. Effectively "dimming" the contents. The inner div is the focused "box". If you have suggestion on a better way to do this, shoot.
src/content/edit-user-script.html
Outdated
| <div rv-hide="modal.errorList | empty"> | ||
| <p>{'script_save_error'|i18n}</p> | ||
| <ul> | ||
| <li rv-each-error="modal.errorList"> |
There was a problem hiding this comment.
This fits so should probably be all on one line <li>...</li>
src/content/edit-user-script.js
Outdated
| event.preventDefault(); | ||
| } | ||
| }); | ||
| rivets.bind(document, gTplData); |
There was a problem hiding this comment.
Should be blank lines, but why did this need to move all the way to the bottom?
There was a problem hiding this comment.
Not really sure why I stuck it all the way at the bottom. Should only need to be below "tplData"
b486cb7 to
1e9433e
Compare
|
Status check on this PR please? |
Should be good. Resolved all the issues in the review. |
580c274 to
1696a9b
Compare
|
Rebased on master |
All required details can be obtained from the downloader.
This also preps the editor for using rivets in the upcoming modal changes.
The modal tracks progress and will display any errors that occur during save. fixes greasemonkey#2847
1696a9b to
ec5afe9
Compare
|
Rebased on master |
see #2858