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.

Sign up for GitHub

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

Jump to bottom

dev/accessibility#10 : Moved datepicker widget to a separate file #12631

Merged
merged 1 commit into from
Aug 10, 2018
Merged

dev/accessibility#10 : Moved datepicker widget to a separate file #12631

merged 1 commit into from
Aug 10, 2018

Conversation

monishdeb
Copy link
Member

Overview

This is a subPR of #12580 and to move datepicker functionality to a separate file

@civibot
Copy link

civibot bot commented Aug 7, 2018

(Standard links)

colemanw
colemanw reviewed Aug 7, 2018
View reviewed changes
js/Common.js Outdated
@@ -646,155 +646,14 @@ if (!CRM.vars) CRM.vars = {};
return combined;
}

function copyAttributes($source, $target, attributes) {
CRM.utils.copyAttributes = function ($source, $target, attributes) {
_.each(attributes, function(name) {
if ($source.attr(name) !== undefined) {
$target.attr(name, $source.attr(name));
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Jenkins says this line is missing a semicolon.

@eileenmcnaughton
Copy link
Contributor

@monishdeb test errors relate

@monishdeb
Copy link
Member Author

I see

@colemanw do I need to include the new file anywhere in test suite, as I checked on the UI where crm-mailing-radio-date directive is used and it's working correctly.

@colemanw
Copy link
Member

colemanw commented Aug 8, 2018

Yes I suspect the new file needs to be included in Karma setup somewhere... but grepping around so far I can't find where.

@seamuslee001
Copy link
Contributor

guessing maybe in this array https://github.com/civicrm/civicrm-core/blob/master/karma.conf.js#L25

@monishdeb
Copy link
Member Author

monishdeb commented Aug 8, 2018 •
edited
Loading

Thanks a lot @seamuslee001 indeed it seems to be the point.

@monishdeb
Copy link
Member Author

Jenkins test this please

1 similar comment
@seamuslee001
Copy link
Contributor

Jenkins test this please

seamuslee001
seamuslee001 reviewed Aug 10, 2018
View reviewed changes
js/crm.datepicker.js
updateInputFields();
});
};
})(jQuery, CRM, CRM._);
Copy link
Contributor

Choose a reason for hiding this comment

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

@monishdeb phantomjs is complaining about not able to find CRM I think the date picker file needs to be loaded after Common.js

@seamuslee001
Copy link
Contributor

Merging as per the tag

@seamuslee001 seamuslee001 merged commit 7060948 into civicrm:master Aug 10, 2018
@monishdeb
Copy link
Member Author

Thanks @seamuslee001 :)

@monishdeb monishdeb deleted the dev-access-10-1 branch August 11, 2018 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seamuslee001 seamuslee001 seamuslee001 left review comments

@colemanw colemanw colemanw left review comments

Assignees
No one assigned
Labels
master merge on pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@monishdeb @eileenmcnaughton @colemanw @seamuslee001 @civicrm-builder