Skip to content
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

Allow binding <select>s via data attributes #257

Merged
merged 1 commit into from
Oct 22, 2014
Merged

Allow binding <select>s via data attributes #257

merged 1 commit into from
Oct 22, 2014

Conversation

bregenspan
Copy link
Contributor

This attempts to resolve #249 by allowing pre-rendered <select> <option>s to have values bound from a data attribute (data-stickit-bind-val, which was previously used only internally and named data-stickit_bind_val), rather than from the <option value>.

The tests now verify that pre-rendered <select>s without data-stickit-bind-val continue to have their options processed to a collection containing string values, and that ones with data-stickit-bind-val now support numeric values.

This fixes a longstanding issue with binding pre-rendered selects at the cost of some extra complexity and a bit of duplicative markup.

@bregenspan
Copy link
Contributor Author

@akre54 would you mind taking another look at this one? Thank you for your feedback yesterday!

binding = binding || {};
namespace = '.stickit.' + model.cid;

binding = binding || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@akre54
Copy link
Contributor

akre54 commented Oct 22, 2014

Minor nits but otherwise looks good. Thanks!

@akre54
Copy link
Contributor

akre54 commented Oct 22, 2014

Squash down to one commit and I'll merge. Thanks again

@bregenspan
Copy link
Contributor Author

@akre54 many thanks for reviewing this PR! Just fixed those last issues + squashed it down!

@bregenspan
Copy link
Contributor Author

Actually -- my apologies, want to squash in one last change (simply changing deepEqual to strictEqual in tests). Not sure why I used deepEqual here!

@bregenspan
Copy link
Contributor Author

(updated that and Travis seems happy about it)

akre54 added a commit that referenced this pull request Oct 22, 2014
…a_attributes

Allow binding <select>s via data attributes
@akre54 akre54 merged commit 8c57f80 into nytimes:master Oct 22, 2014
akre54 added a commit that referenced this pull request Nov 25, 2014
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.

Binding select box to numeric value
2 participants