Skip to content

feat(ng-if): an implementation of ng-if#317

Closed
marclaval wants to merge 1 commit into
angular:masterfrom
marclaval:ngif
Closed

feat(ng-if): an implementation of ng-if#317
marclaval wants to merge 1 commit into
angular:masterfrom
marclaval:ngif

Conversation

@marclaval
Copy link
Copy Markdown
Contributor

@marclaval marclaval commented Dec 17, 2014

  • adds ng-if to the directives module

@googlebot
Copy link
Copy Markdown

googlebot commented Dec 17, 2014

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

mhevery
mhevery reviewed Jan 6, 2015
View reviewed changes
Comment thread modules/directives/src/ng_if.js Outdated
Copy link
Copy Markdown
Contributor

@mhevery mhevery Jan 6, 2015

Choose a reason for hiding this comment

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

Change this to setter

set condition(newValue) {...}

then delete the onChange method as it will not be needed. This will also be more efficient.

@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Jan 6, 2015

Thanks for the PR. Very nice work, which is appreciated! Please look at our comments and do some cleanup.

@mhevery mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 6, 2015
@marclaval
Copy link
Copy Markdown
Contributor Author

marclaval commented Jan 7, 2015

Thanks for the review, I've updated the PR accordingly.

Because of the removal of toBool(), I now have JS and Dart specific tests. I'm using the IS_DARTIUM constant to differentiate the cases, but it could also be done with separate files. What would be the strategy please ?

@marclaval marclaval force-pushed the ngif branch 2 times, most recently from 7b34bf1 to af22d38 Compare January 8, 2015 09:04
@vicb
Copy link
Copy Markdown
Contributor

vicb commented Jan 8, 2015

I think the best is to keep all the tests for a directive in a single file (i.e. use IS_DARTIUM)

@mhevery mhevery added cla: yes and removed cla: no labels Jan 22, 2015
@mhevery mhevery closed this in d5a12d5 Jan 22, 2015
@marclaval marclaval deleted the ngif branch December 11, 2017 10:14
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Sep 13, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@marclaval @googlebot @mhevery @vicb @vsavkin @naomiblack