Skip to content

Conversation

robertkowalski
Copy link
Member

To not render a property in React we need to pass null to it. If
we are passing false or "false" it gets rendered into the DOM.

This fixes a bug where you edited a document and then got no
confirmation dialog if you are really sure you want to leave the
page without saving if you clicked on the navigation bar on the
left.

Having data-bypass="false" will mean to the selector
:not(["data-bypass"]) that there is a data-bypass - containing
a String! (which then matches the selector)

What we want is no data-bypass if we don't want to bypass.

Looking at the old DOM a month ago shows how we used data-bypass:

bildschirmfoto 2015-02-12 um 19 12 19

Closes COUCHDB-2574

superseeds #263

@robertkowalski robertkowalski force-pushed the 2574-save-editor-changed-confirmation-react-bug branch from 60dce88 to 667fee7 Compare February 12, 2015 18:43
Fix for passing false as a string with is evaluated truthy

This fixes a bug where you edited a document and then got no
confirmation dialog if you are really sure you want to leave the
page without saving if you clicked on the navigation bar on the
left.

To not render a property in React we need to pass `null` to it. If
we are passing `false` or `"false"` it gets rendered into the DOM.

Having `data-bypass="false"` will mean to the selector
`:not(["data-bypass"])` that there is a `data-bypass` - containing
a String! (which then matches the selector)

What we want is _no_ `data-bypass` if we don't want to bypass.

Closes COUCHDB-2574
@robertkowalski robertkowalski force-pushed the 2574-save-editor-changed-confirmation-react-bug branch from 667fee7 to 1e0766e Compare February 12, 2015 18:45
@benkeen
Copy link
Member

benkeen commented Feb 12, 2015

Awesome. +1

I still think #263 should be added, because it's a more thorough fix.

The currently logic is incorrect: it just checks for the existence of the data-bypass attribute and not its value (true/false). As such, it will never run the event delegated code for anything with that attribute regardless of value. To put it another way, right now these two are equivalent:

<a href="#abc" data-bypass="true">...</a>
<a href="#abc" data-bypass="false">...</a>

And I think that this should be considered a bug. The first case clearly (to me) implies the link will bypass something; the second, not. However, both cause the link click to be bypassed.

Your fix fixes it for the primary nav React components, but not for any other code. #263 gets to the root cause and will fix it for the entire application.

Just my 2 cents.

@robertkowalski
Copy link
Member Author

@benkeen I like the idea of #263 as it follows the principles of the least surprise.

What do you think of something like this for changing the routers api: match on all, but not on data-bypass="false". This would also keep older components like the global notifications working.

@robertkowalski robertkowalski deleted the 2574-save-editor-changed-confirmation-react-bug branch February 17, 2015 12:48
@robertkowalski
Copy link
Member Author

merged as b030b21 and c6e6a43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants