Skip to content
This repository has been archived by the owner. It is now read-only.

Adds the ability to conditionally hide fields. #17

Merged
merged 8 commits into from Jun 10, 2014

Conversation

@kentcdodds
Copy link
Member

commented Jun 1, 2014

Closes #16 on original repo. The behavior is completely controlled by the user of formly and formly has no insight into what the value of this field.hide property is.

@GrantCodesCodes

This comment has been minimized.

Copy link
Collaborator

commented Jun 1, 2014

This looks great, do you mind updating the newly minted docs as well. Thanks!

@kentcdodds

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2014

Will get to this after the weekend. Thanks!

  • Kent C. Dodds
    Sent from my mobile device, please forgive any errors or brevity. (I may
    have used speech to text...)
    On May 31, 2014 11:21 PM, "Grant Helton" notifications@github.com wrote:

This looks great, do you mind updating the newly minted docs as well.
Thanks!


Reply to this email directly or view it on GitHub
https://github.com/nimbly/angular-formly/pull/17#issuecomment-44767982.

@kentcdodds

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2014

@astrism I need to run the build and commit those files as well correct?

@kentcdodds

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2014

Alright my changes are in, complete with updated README, bower version, CHANGELOG, and demo. Thanks!

Perhaps eventually we could make it as simple as what I wanted to do (as illustrated by #16)...

@kentcdodds

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2014

I just found a better solution. Let me make another pull request and we can talk about it. It actually enables both options so I'll probably just include it in this PR. Give me just a second...

@kentcdodds

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2014

@astrism, let me know if you have any questions. But this seems to work quite nicely

@GrantCodesCodes

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2014

Hey Kent, The first part of your pull request for hide looks great, but I'm wondering if there is a more efficient way to handle the watches. I'm seeing 6 calls to the watch method in its current implementation no matter what is changed on the scope. I believe its 2 calls per hideExpression, per scope change of any kind to the form scope, not just the results field. I don't think this would be a major bottle neck, but I spent a little time trying a few options to see if we could get it down.

In addition, the watcher code only runs when the form is instantiated so if you modify the form definition it won't rerun and the functionality is broken. Editing the form definition is an edge case, but I'd like to create some sort of form construction tool in the future using this library, so I see that being an issue as well.

I tried this the code below, it just watched result, deeply, and if there are changes it loops through the fields to see if they have hide expressions. I don't think its the most elegant solution, but it does work around those two problems listed above. What do you think, I'm open for discussion! My only concern with my solution is looping through all the fields every time...

    //formly-form.js
    ...
controller: function($scope, $element) {
            $scope.$watch('result', function(newValue) {
                console.log('result changed');
                angular.forEach($scope.fields, function(field, index) {
                    if (field.hideExpression) {
                        var getter = $parse(field.hideExpression);
                        field.hide = getter($scope.result);
                    }
                });
            }, true);
        },
    ...
@kentcdodds

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2014

Just throwing this out there... Would it be more performant if we put an ng-change on the formly-field directive? This way the watch listener is only executed when a change occurred and the watch expression is not evaluated.

Either way, I don't see a way of getting around a dynamic form definition without doing things this way. And I imagine if this goes the direction you're wanting it to, you're going to have to loop through each field every time the result changes anyway. I think the solution you have above would work well... Let me know if you want me to change my pull request to reflect this.

@kentcdodds

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2014

@astrism, I would really love it if we could figure out what needs to happen for this to get merged today. I have another PR I'm creating in a little bit that I would like in, but I don't want to update the changelog for it because we'd have a merge conflict with this PR. Let me know what I need to do to get this in.

GrantCodesCodes added a commit that referenced this pull request Jun 10, 2014

Merge pull request #17 from kentcdodds/master
Adds the ability to conditionally hide fields.

@GrantCodesCodes GrantCodesCodes merged commit 1447314 into formly-js:master Jun 10, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.