Skip to content

Conversation

@simsalabim
Copy link
Contributor

Hi all,

It seems sort of weird that link/form/btn which has "data-remote"="false" is treated as if it had "data-remote"="true" (or just "data-remote"). Taking into account that we regularly do use ujs functionality with rails helpers, almost everyone passes a value to the data-remote attribute.

It will be useful in such cases as the following (simplified example):

- @links.each do |link|
  = link_to link.name, link.href, data: { remote: link.remote? }

Despite the fact the case I stumbled upon is quite rare, having ajax proceeding with "data-remote"="false" seems weird anyway.

@JangoSteve
Copy link
Member

I think I'd rather stick with the principle of least surprise here. When you have HTML5 data attributes (or really just any attributes in general), there's no such thing as a false value; since the values are considered strings, falsey would be leaving it absent or undefined. I'd prefer not to have code in UJS where we're defining specific non-blank strings to be falsey.

That being said, I see the value, and I think I'd be fine with it if there weren't another really easy way to accomplish this already. Instead of using data: { remote: link.remote? }, you should use the actual rails way of just remote: true.

- @links.each do |link|
  = link_to link.name, link.href, remote: link.remote?

It's actually shorter and easier than what you have. If you use the built-in rails helper, it's smart enough to exclude the data-remote attribute from the HTML when you pass remote: false.

@simsalabim
Copy link
Contributor Author

@JangoSteve thanks for the reply. The principle of least surprise is the thing urged me to send this pull request.
This is not about HTML5 only, but about interaction between HTML5 and Javascript.

And as of link.data('remote') returns Object, not String, and in particular if property was set to "false" it returns Boolean, I am convinced that handling it the right way would be to respect such falsy attributes.

I can add that the app I'm working on toggles data-remote on client side as well, and if in Rails we have such a smart helper, javascript is the final frontier and toggling attribute to true/false is more intuitive rather than adding/removing it.

But I hope that the 2nd paragraph is quite powerful argument :)

Regards,
Alex.

@simsalabim
Copy link
Contributor Author

Just to be clear - @JangoSteve's expectations (from the linked discussion):

<a data-remote></a> <!-- old behavior, link is remote -->
<a data-remote=true></a> <!-- old behavior, link is remote -->
<a data-remote=false></a> <!-- new behavior, link is not remote -->

All the expectations are met, corresponding tests pass.

@simsalabim simsalabim force-pushed the feature/falsy-data-remote-attribute branch 3 times, most recently from 9d1b175 to e9b99b9 Compare May 21, 2015 14:46
…d").data("remote") === false` if input's data-remote is set to string "false" (`<input id="my_id" data-remote="false" />`)
@simsalabim simsalabim force-pushed the feature/falsy-data-remote-attribute branch from e9b99b9 to a940d59 Compare May 21, 2015 14:49
@simsalabim
Copy link
Contributor Author

Bump. Rebased from latest master.

@rafaelfranca please, take a look. All tests pass.
I think this is still a valid contribution :)

src/rails.js Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we only need:

// Checks "data-remote" if true to handle the request through a XHR request.

@rafaelfranca
Copy link
Member

Sorry for the delay. I made some few comments, otherwise looks good.

@simsalabim
Copy link
Contributor Author

@rafaelfranca made suggested changes and CI is 💚

rafaelfranca added a commit that referenced this pull request Jun 27, 2015
…ribute

Don't fire ajaxyness if "data-remote"="false"
@rafaelfranca
Copy link
Member

Merged at 99c798d. Thank you

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.

3 participants