You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@couchdb.apache.org by benkeen <gi...@git.apache.org> on 2015/02/10 20:08:51 UTC
[GitHub] couchdb-fauxton pull request: Ensure save changes conf appears
GitHub user benkeen opened a pull request:
https://github.com/apache/couchdb-fauxton/pull/263
Ensure save changes conf appears
Extra eyes on this ticket are appreciated! Bit of explanation
needed.
Whenever you make a change to content in an Ace editor then
navigate away, you should see a confirmation alert saying "Are you
sure you don't want to save these changes?". Right now this
appears sometimes, but not always. This ticket makes a small
change to the main event delegation function in main.js that runs
on all <a> tags. Before, as long as the <a> didn't contain a
data-bypass attribute, it did a FauxtonAPI.navigate call, which
meant that all beforeUnload functions got ran prior to redirecting
- and that was where the confirmation alert appeared.
This has now been changed to only NOT run when the data-bypass
attribute is set to true, which I think is what was originally
intended. This small change will ensure any pages containing the
Ace editor will have their beforeUnload functions ran prior to
redirecting from any link.
Closes COUCHDB-2574
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/benkeen/couchdb-fauxton 2574-save-editor-changed-confirmation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-fauxton/pull/263.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #263
----
commit c29562a2da382b15ba4c59f487835fdd6974c435
Author: Ben Keen <be...@gmail.com>
Date: 2015-02-10T18:59:41Z
Ensure save changes conf appears
Extra eyes on this ticket are appreciated! Bit of explanation
needed.
Whenever you make a change to content in an Ace editor then
navigate away, you should see a confirmation alert saying "Are you
sure you don't want to save these changes?". Right now this
appears sometimes, but not always. This ticket makes a small
change to the main event delegation function in main.js that runs
on all <a> tags. Before, as long as the <a> didn't contain a
data-bypass attribute, it did a FauxtonAPI.navigate call, which
meant that all beforeUnload functions got ran prior to redirecting
- and that was where the confirmation alert appeared.
This has now been changed to only NOT run when the data-bypass
attribute is set to true, which I think is what was originally
intended. This small change will ensure any pages containing the
Ace editor will have their beforeUnload functions ran prior to
redirecting from any link.
Closes COUCHDB-2574
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Ensure save changes conf appears
Posted by robertkowalski <gi...@git.apache.org>.
Github user robertkowalski commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/263#issuecomment-74057182
I am not sure why this would fix the issue - the cancel button in the editor does not use a `data-bypass` and changing the selector does not change the way event delegation in jQuery works right now. Can you explain how this would remove the race condition you describe and how the race condition looks like?
After further investigation I think this was fixed in https://github.com/apache/couchdb-fauxton/pull/232 - I can't reproduce the issue on the fresh master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Ensure save changes conf appears
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen closed the pull request at:
https://github.com/apache/couchdb-fauxton/pull/263
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Ensure save changes conf appears
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/263#issuecomment-74333419
Closing. #267 fixes the observed issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Ensure save changes conf appears
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/263#issuecomment-74104710
Hi @robertkowalski thanks for looking at this. That's correct: the issue with the back was already fixed.
Did you try the steps that I outlined above? This should fix the problem for clicking on any other link in the page that contains a `data-bypass` attribute.
Maybe let's talk in person, I'm not sure how else to describe this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Ensure save changes conf appears
Posted by garrensmith <gi...@git.apache.org>.
Github user garrensmith commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/263#issuecomment-73850525
I'm not sure about this. This is where we check the [custom beforeUnloads](https://github.com/apache/couchdb-fauxton/blob/master/app/core/router.js#L26-L47). Then we also listen to the [window.beforeUnload](https://github.com/apache/couchdb-fauxton/blob/master/app/addons/fauxton/components.js#L821-L825). So I'm not convinced your fix would make much difference.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Ensure save changes conf appears
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/263#issuecomment-73905992
Yes, but the beforeUnload code you noted only gets executed if a .navigate() fires; if an `<a>` just ends up firing its `href="#whatever"` the before unload methods don't get called.
Here's some steps to test this ticket:
- open up the full page editor on any doc
- make a small change
- click something in the primary nav (e.g. Active Tasks).
In the current build, it just redirects there. With this ticket, it prompts you to save.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---