Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

ensured the scheduler input checked attribute is being set, not just setting the active css class #1074

Merged
merged 4 commits into from
Feb 12, 2015

Conversation

mbeard
Copy link
Contributor

@mbeard mbeard commented Feb 10, 2015

fixes #992

@swilliamset
Copy link
Contributor

@mbeard travis is failing on test failing

@mbeard mbeard self-assigned this Feb 11, 2015
@mbeard mbeard added this to the 3.6.0 milestone Feb 11, 2015
@@ -515,7 +515,7 @@
item.find('label').removeClass('active');
temp = recur.BYDAY.split(',');
for (i = 0, l = temp.length; i < l; i++) {
item.find('input[data-value="' + temp[i] + '"]').parent().addClass('active');
item.find('input[data-value="' + temp[i] + '"]').attr('checked',true).parent().addClass('active');

Choose a reason for hiding this comment

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

It doesn't look like your unit test is checking for the checked attr to be present and true, is this on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was intending to test getValue which in turn tests this indirectly. I will create a separate unit test to explicitly test the checked attribute.

Choose a reason for hiding this comment

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

I believe this should be .prop('checked', true) not attr('checked', true). http://api.jquery.com/prop/

the most important concept to remember about the checked attribute is that it does not correspond to the checked property. The attribute actually corresponds to the defaultChecked property and should be used only to set the initial value of the checkbox. The checked attribute value does not change with the state of the checkbox, while the checked property does.

@cmcculloh-kr cmcculloh-kr assigned cmcculloh-kr and unassigned mbeard Feb 11, 2015
@cmcculloh-kr
Copy link

OK, I can confirm that .attr('checked') is not getting set "under the covers":

image

But if we set it, it will be set:

image

and furthermore, if we use attr() instead of prop(), despite what the jquery docs tell us, it appears to work better:

image

@mbeard
Copy link
Contributor Author

mbeard commented Feb 11, 2015

Why not rely on our defined API for interrogating the scheduler vs. supporting inspecting the classnames, attributes, etc. on the DOM elements of the scheduler?

The latter seems to be problematic and cause for future issues.

cmcculloh-kr pushed a commit that referenced this pull request Feb 12, 2015
ensured the scheduler input checked attribute is being set, not just setting the active css class
@cmcculloh-kr cmcculloh-kr merged commit c1765c5 into ExactTarget:master Feb 12, 2015
@cmcculloh-kr
Copy link

This has possible ramifications for anyone who was incorrectly accessing the value using attr instead of interrogating our API using getValue or change event (or, worst case, using prop)

@interactivellama
Copy link
Contributor

We have something in that essentially said we will legacy support our API and initial markup if we change it, custom DOM interrogating though may break, but I can't find it now.

Here it is. It's only about events though.

Listening for standard and Bootstrap supported events on an internal element is not tested, and therefore not recommended.

We could add a disclaimer for markup and/or jQuery selectors inspecting into the control, etc.

@swilliamset
Copy link
Contributor

Should we add access attributes directly from markup to this? This isn't the first time we've ran into attributes not being synced. It's a common problem with jQuery objects.

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

Successfully merging this pull request may close these issues.

Scheduler reoccurance pattern loses weekday
4 participants