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

CRM-21497 - crmRouteBinder: add deep comparison option #11345

Merged
merged 1 commit into from
Nov 30, 2017

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Nov 29, 2017

Overview

This PR updates the crmRouteBinder angular directive to optionally perform deep comparisons on collections. It updates the docs as well.

Before

$watchCollection was used to watch arrays and objects which does not detect changes more than 1 level deep.

After

Optionally passing deep: true will use $watch in strict comparison mode. This is more memory intensive but the only way to keep track of nested objects.

@colemanw
Copy link
Member Author

@seanmadsen maybe these docs belong elsewhere?

@colemanw colemanw merged commit 74fb972 into civicrm:master Nov 30, 2017
@colemanw colemanw deleted the CRM-21497 branch November 30, 2017 15:42
@seancolsen
Copy link
Contributor

@colemanw Cool. Thanks for the ping. I only have a minute so apologies that this answer won't be as thorough as perhaps it should.

  • On first glance I would say that this content looks like it belongs in code-level docs and not in the Dev Guide.
  • The .md file alongside the .js is certainly better than nothing, so thanks for making that.
  • Earlier this year I noticed how poorly some of our AngularJS code is documented and I spent a little time researching how AngularJS writes its own code-level docs. I posted some of these thoughts in Mattermost at the time, but I think the topic warrants some more attention in general. For PHP code, our practice is very clear and straightforward — use phpdoc style docblocks. For AngularJS, the process for writing code-level docs is less clear, and I think we would benefit from adopting some standards in this area.

So in summary: what you've done seems decent. But I think it could be better if it were structured more in line with the way that AnguarJS documents their code (by using docblock-like comments with markdown embedded)... but also I recognize that as a community we have not established an expectation for developers to use this type of doc. And also take this all with a grain of salt because I've only skimmed what you've done here!

sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
CRM-21497 - crmRouteBinder: add deep comparison option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants