-
Notifications
You must be signed in to change notification settings - Fork 3k
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(uiState): add ui-state directive #1932
Conversation
* @restrict A | ||
* | ||
* @description | ||
* Much like ui-sref, but will accept named $scope propertys to evaluate for a state definition, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/propertys/properties/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll get on it asap, thanks for the catch :)
Hey, thanks for pulling this together! I made a couple of comments, but barring those it looks good to me. @christopherthielen Any comments? |
I like the effort to reuse the existing ui-sref code for ui-sref-active, (hopefully that works). I'll have to play with this for a while before I can comment on the implementation. |
Hmph. No sauce. evaluating the full array will always return undefined. I can think of a couple of ways to achieve the expected result and move away from concatenation: return [scope.$eval(attrs.uiState), scope.$eval(attrs.uiStateParams), scope.$eval(attrs.uiStateOpts)];
return [scope[attrs.uiState], scope[attrs.uiStateParams], scope[attrs.uiStateOpts]];
return [attrs.uiState, attrs.uiStateParams, attrs.uiStateOpts].map(function (prop) {
return scope.$eval(prop);
}); thoughts? |
Yeah, I just realized I was looking at it wrong... hmm...
I'm not so much worried about the syntax as I am about the performance. Trying to think of how we can get it down to one I'll test it out when I have time, unless you beat me to it. |
I'll see if I can come up with some way of cutting it down to a single $eval, not sure what that would look like though. Regarding |
Reverted back to using 3x |
Works much like ui-sref but with a slight difference in that it accepts named $scope properties to evaulate for a state ref. Closes #395
Unfortunately I have still not been able to figure out a way to run a single $eval call for three separate expressions - without parsing possible objects into JSON and then concatenating the strings. I did clean up the code ever so slightly in the return [
scope.$eval(attrs.uiState),
scope.$eval(attrs.uiStateParams),
scope.$eval(attrs.uiStateOpts)
]; Into: return [
attrs.uiState,
attrs.uiStateParams,
attrs.uiStateOpts
].map(scope.$eval.bind(scope)); Opted for One could argue that it "helps" with ES5+IE8, but that is sort of moot seeing as how |
@nateabele @christopherthielen - What are your thoughts on the current state of things in this PR? Have you gotten a chance to play around with it? Personally, I will probably never use I do share your concern in regards to multiple calls to |
Refactored this into #2187. |
Closing this in favour of #2187. |
- Refactor StateRefDirective for better modularity - Drop key restrictions on ui-sref-opts - Improves performance over prior implementation with no extra $eval()’s Fixes angular-ui#395, angular-ui#900, angular-ui#1932
ui-sref with dynamic state/option/param pointers.
Usage:
The resulting
href
would be pointed tomyStateName({ id: 5 })
and$state.go
would be called withreload: true
.It's also possible to specify the params straight into the
ui-state
directive as such:I started off writing this as a completely standalone directive, but given that it is desirable for it to work much like
ui-sref
, I felt there was too much duplication going on.ui-state
can therefore be seen as a wrapper around the behaviour ofui-sref
, nothing more to it really.Now, this thing is heavy on the
$eval
and slightly so on the$watch
side of things, so if someone has any nifty ideas on how to make that performant (I have not tested the performance aspects of ui-state in depth), that'd be super sweet.Do let me know if you think there's a test missing among the added ones - I can't say I'm 100% certain that I've tested every aspect of a 'dynamic ui-sref directive'.
Reference issues: #395 #900
Suggested implementation