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

feat(bhCurrencySelect): currency selection radios #178

Closed

Conversation

jniles
Copy link
Collaborator

@jniles jniles commented Mar 7, 2016

This commit introduces the bhCurrencySelect radio component to select
currencies in forms. The basic signature looks like this:

 <!-- simple usage -->
 <bh-currency-select
   currency-id="ParentCtrl.model.currencyId"
   validation-trigger="ParentForm.$submitted"
   >
 </bh-currency-select>

 <!--
   complex usage: disable currencies based on ids found in the
   disable-ids array and register an ngChange event.
 -->
 <bh-currency-select
   currency-id="ParentCtrl.model.currencyId"
   on-change="ParentCtrl.currencyChangeEvent()"
   disable-ids="ParentCtrl.disabledIds"
   validation-trigger="ParentForm.$submitted"
   >
 </bh-currency-select>

See the source code for implementation details. The list of supported
bindings follows:

  1. [currency-id] - the model value for the underlying <input>s. This is two-way bound to the parent controller.
  2. [validation-trigger] (optional) - a boolean that can be passed in to show validation messages will only show if this boolean is true. It is useful to bind ParentForm.$submitted value to this attribute.
  3. [on-change] (optional) - a callback bound the ng-change event on the <input>s.
  4. [disable-ids] (optional) - an array of currency ids to be disabled as required.

An implementation of the most complex case is given in the cash module, with passing end to end tests.

There is also a currency component wrapper that works as you would expect:

var components = require('../shared/components');

it('works like a charm!', function () {

  // set currency input to currencyId = 1 (FC)
  components.bhCurrencySelect.set(1);
});

it('supports an id lookup!', function () {

  // set the currency input with id='outgoing' to USD
  components.bhCurrencySelect.set(2, 'outgoing');

  // set the currency input with id='incoming' to USD
  components.bhCurrencySelect.set(2, 'incoming');
});

Partially addresses #106.

}
});

bhCurrencySelect.$inject = [ '$scope', 'CurrencyService' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't inject the all scope, the component should not access the whole scope.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I use $scope.$watch(). Can you give some example code that does the same thing and does not use $scope.$watch()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DedrickEnc, could you please give me feedback on how to make this code better? I do not know a better way than this. What suggestions do you have?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jniles , I have an idea and I think it good.
When reading the component documentation, I understood that, the component is only responsible of his own view, that is why is scope is always isolated.
In theory you can access the entire view of the page where a component is injected by using scope and watch but this is a bad practice.
When improving currency input, I faced the same problem, I was supposed to use scope but I have an other alternative, using the component tree design.
So for the currency input for exemple, I will need the currency select first, so when a user change the currency, the currency select component will call an appropriate method of currency input. so the currency input will update his view and apply specific validation rule.
I will use your code (currency select) to perform it. And will submit the PR this evening, so you will review it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You do realize that $scope isn't the entire $scope of the page right? It is an isolated scope. In this component's case, the only thing attached to the $scope is the child form found inside the component.

I'll be interested to see your design.

This was referenced Mar 7, 2016
@jniles jniles force-pushed the component-bh-currency-select branch 2 times, most recently from c4d7f13 to f2b86bd Compare March 11, 2016 11:02
@jniles
Copy link
Collaborator Author

jniles commented Mar 11, 2016

So, after some research, here was I have discovered about AngularJS's scopes.

  1. All components create an isolated $scope. This does not mean that they do not have access to the $parent scope, it simply means that they do not prototypically inherit from their parent scope. This prevents failed local property/method lookups from propagating up the $scope chain.
  2. Angular uses $watch() for all change detection or dirty checking. If you want to know if something's changed, either you, the developer, or angular itself will set up a $watch() somewhere.
  3. This PR should really use $scope.$watchCollection(), since it doesn't need to know if subproperties or the array change. This would be more efficient.

Honestly, I do not think the $scope.$watch() can be replaced in this case, except by $watchCollection().

This is not the only way to achieve the functionality of disabling ids. Here is are some other ways of doing this:

  1. $scope.$on()/$scope.$broadcast() - still uses $scope, probably a little more efficient, but requires that both the parent and the child import and use $scope.
  2. Build a custom service to allow disabling ids in the component. The component would pull its currencies from this service, instead of $watch()ing the external bindings. The disadvantage is that it would dramatically change the way the component operates for all modules, instead of just allowing a custom attribute for this specific case.

Sources
Angular guide on $scope.
Angular $compile API

This commit introduces the bhCurrencySelect radio component to select
currencies in forms.  The basic signature looks like this:
```html
 <!-- simple usage -->
 <bh-currency-select
   currency-id="ParentCtrl.model.currencyId"
   validation-trigger="ParentForm.$submitted"
   >
 </bh-currency-select>

 <!--
   complex usage: disable currencies based on ids found in the
   disable-ids array and register an ngChange event.
 -->
 <bh-currency-select
   currency-id="ParentCtrl.model.currencyId"
   on-change="ParentCtrl.currencyChangeEvent()"
   disable-ids="ParentCtrl.disabledIds"
   validation-trigger="ParentForm.$submitted"
   >
 </bh-currency-select>
```
See the source code for implementation details.  The list of supported
bindings follows:
 1. [currency-id] - the model value for the underlying `<input>`s.  This
 is two-way bound to the parent controller.
 2. [validation-trigger] (_optional_) - a boolean that can be passed in
 to show validation messages will only show if this boolean is true.  It
 is useful to bind `ParentForm.$submitted` value to this attribute.
 3. [on-change] (_optional_) - a callback bound the `ng-change` event on
 the `<input>`s.
 4. [disable-ids] (_optional_) - an array of currency ids to be disabled
 as required.

 An implementation of the most complex case is given in the cash module.

 Partially addresses #106.
@jniles jniles force-pushed the component-bh-currency-select branch from f2b86bd to 4c44179 Compare March 11, 2016 13:42
@jniles
Copy link
Collaborator Author

jniles commented Mar 14, 2016

Since I've received no feedback on why this is a bad design and no reference documentation to where someone recommends against using $scope.$watch in this scenario, I'm going to close this.

@jniles jniles closed this Mar 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants