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

Use save modal with editor #2900

Merged
merged 12 commits into from Jun 7, 2018

Conversation

Projects
None yet
2 participants
@Sxderp
Contributor

Sxderp commented Mar 10, 2018

see #2858

@arantius

This comment has been minimized.

Collaborator

arantius commented Mar 13, 2018

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.)

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Mar 13, 2018

While I can see your point and I have some ideas on how to implement those checks (may require reworking of Downloader depending on how 'smart' the checks are). There's still the problem that we have no idea how long a "save" will take. I don't know how normal text editors deal with this problem (obviously not all of them save synchronously) but I can't think of any sane way (caching entire contents being the only idea I have) to deal with little things like marking the editor as "clean." You make some changes, save, saving takes a while and during that time you make more changes, the save completes, how do we figure out whether to mark the editor as "clean."?

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.

@arantius

This comment has been minimized.

Collaborator

arantius commented Mar 14, 2018

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.

@arantius arantius added this to the 4.4 milestone Mar 14, 2018

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Mar 14, 2018

I can't think of another text editor that ever has any kind of progress/dialog as part of a save action

I've seen editors that save synchronously. Most of the time the editor just freezes rather than bring up a dialog box.

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Mar 31, 2018

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.).

body.save #modal
{ display: grid; }

This comment has been minimized.

@arantius

arantius Apr 2, 2018

Collaborator

Rarely all on one line, otherwise always:

selector {
  rules
}
@@ -63,3 +118,6 @@ body, #editor {
.CodeMirror {
height: 100% !important;
}
.CodeMirror pre {
line-height: 14px !important;

This comment has been minimized.

@arantius

arantius Apr 2, 2018

Collaborator

Why? Does this have anything to do with showing a modal dialog? Some side effect of now including shoelace?

This comment has been minimized.

@Sxderp

Sxderp Apr 2, 2018

Contributor

Slide effect of shoelace.

</ul>
</div>
</div>
<div class="panel-foot">

This comment has been minimized.

@arantius

arantius Apr 2, 2018

Collaborator

Just <footer>?

@@ -39,6 +40,36 @@
</head>
<body>
<div id="modal">
<div class="panel">
<div class="panel-head">

This comment has been minimized.

@arantius

arantius Apr 2, 2018

Collaborator

Just <header>?

@@ -39,6 +40,36 @@
</head>
<body>
<div id="modal">
<div class="panel">

This comment has been minimized.

@arantius

arantius Apr 2, 2018

Collaborator

Why do I need an extra wrapper div here?

This comment has been minimized.

@Sxderp

Sxderp Apr 2, 2018

Contributor

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.

<div rv-hide="modal.errorList | empty">
<p>{'script_save_error'|i18n}</p>
<ul>
<li rv-each-error="modal.errorList">

This comment has been minimized.

@arantius

arantius Apr 2, 2018

Collaborator

This fits so should probably be all on one line <li>...</li>

@@ -173,3 +217,4 @@ window.addEventListener('beforeunload', event => {
event.preventDefault();
}
});
rivets.bind(document, gTplData);

This comment has been minimized.

@arantius

arantius Apr 2, 2018

Collaborator

Should be blank lines, but why did this need to move all the way to the bottom?

This comment has been minimized.

@Sxderp

Sxderp Apr 2, 2018

Contributor

Not really sure why I stuck it all the way at the bottom. Should only need to be below "tplData"

@arantius

This comment has been minimized.

Collaborator

arantius commented May 3, 2018

Status check on this PR please?

@arantius arantius modified the milestones: 4.4, 4.5 May 3, 2018

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented May 4, 2018

Status check on this PR please?

Should be good. Resolved all the issues in the review.

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented May 20, 2018

Rebased on master

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Jun 1, 2018

Rebased on master

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

@arantius arantius merged commit ec5afe9 into greasemonkey:master Jun 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment