Skip to content

First pass at state directive for auto-generating link URLs #139

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jun 22, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ module.exports = function (grunt) {
'src/urlRouter.js',
'src/state.js',
'src/viewDirective.js',
'src/stateDirectives.js',
'src/compat.js'
],
dest: '<%= builddir %>/<%= pkg.name %>.js'
Expand Down
11 changes: 8 additions & 3 deletions src/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -266,9 +266,14 @@ function $StateProvider( $urlRouterProvider, $urlMatcherFactory) {
return $state.$current.includes[findState(stateOrName).name];
};

$state.href = function (stateOrName, params) {
var state = findState(stateOrName), nav = state.navigable;
if (!nav) throw new Error("State '" + state + "' is not navigable");
$state.href = function (stateOrName, params, options) {
options = extend({ lossy: true }, options || {});
var state = findState(stateOrName);
var nav = options.lossy ? state.navigable : state;

if (!nav || !nav.url) throw new Error("State '" + state + "' " + (
options.lossy ? "does not have a URL or navigable parent" : "is not navigable"
));
return nav.url.format(normalize(state.params, params || {}));
};

Expand Down
46 changes: 46 additions & 0 deletions src/stateDirectives.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
function parseStateRef(ref) {
var parsed = ref.match(/^([^(]+?)\s*(\((.*)\))?$/);
if (!parsed || parsed.length !== 4) throw new Error("Invalid state ref '" + ref + "'");
return { state: parsed[1], paramExpr: parsed[3] || null };
}

$StateRefDirective.$inject = ['$state'];
function $StateRefDirective($state) {
return {
restrict: 'A',
link: function(scope, element, attrs) {
var ref = parseStateRef(attrs.uiSref);
var params = null, url = null;
var isForm = element[0].nodeName === "FORM";
var attr = isForm ? "action" : "href", nav = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there actually any meaningful way to use ui-sref with forms at the moment? Maybe we should remove form support if there isn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, forms and links should always have valid URLs whenever possible to maintain compatibility with browsers and browser tooling.


var update = function(newVal) {
if (newVal) params = newVal;
if (!nav) return;

try {
element[0][attr] = $state.href(ref.state, params, { lossy: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

lossy is already the default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it better to make that explicit, especially since there was discussion around reversing that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you wrapping this in a try block because href() may throw an error? I'm wondering if it would be more helpful if href() returned undefined or maybe "#" on invalid states? Devs might use the method in a loop through states that may or may not have urls, might be annoying to always have to wrap it in a try block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I think null would probably be best then.

Copy link
Contributor

Choose a reason for hiding this comment

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

That works too

Copy link
Contributor

Choose a reason for hiding this comment

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

Right ok. Hmmm. I think it would be more convenient as null in use. It
doesn't seem fatal enough to throw an error in my opinion. I wonder what
@ksperling thinks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the consensus is that null is the way to go, that's fine with me. @ksperling?

Choose a reason for hiding this comment

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

This is exciting. I hope sref can degrade gracefully if it is used in
unexpected places or can't do its job due to bad state definitions (or
whatever) as would fit html norms.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess returning null from href() seems OK for a non-navigable state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even though I generally also like things to fail at the point where something is actually wrong, instead of silently passing nulls or other invalid data around and having it blow up later at a completely unrelated place. But in this specific case it's probably benign enough.

} catch (e) {
nav = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be nicer to check once at the beginning of link() if the target state is navigable and either fail if it isn't or disable the directive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not to disable the directive, because you can still validly use it even without a URL (since the click handler uses transitionTo()).

As far as checking it at the beginning, it would be awkward to move that code around I think, and IMO of no benefit, since nav already prevents repeated attempts to generate an HREF.

}
};

if (ref.paramExpr) {
scope.$watch(ref.paramExpr, function(newVal, oldVal) {
if (newVal !== oldVal) update(newVal);
}, true);
params = scope.$eval(ref.paramExpr);
}
update();

if (isForm) return;

element.bind("click", function(e) {
$state.transitionTo(ref.state, params);
e.preventDefault();
});
}
};
}

angular.module('ui.state').directive('uiSref', $StateRefDirective);
70 changes: 70 additions & 0 deletions test/stateDirectivesSpec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
describe('uiStateRef', function() {

beforeEach(module('ui.state'));

beforeEach(module(function($stateProvider) {
$stateProvider.state('index', {
url: '/'
}).state('contacts', {
url: '/contacts'
}).state('contacts.item', {
url: '/:id'
}).state('contacts.item.detail', {});
}));

describe('links', function() {
var el, scope;

beforeEach(inject(function($rootScope, $compile) {
el = angular.element('<a ui-sref="contacts.item.detail({ id: contact.id })">Details</a>');
scope = $rootScope;
scope.contact = { id: 5 };
scope.$apply();

$compile(el)(scope);
scope.$digest();
}));


it('should generate the correct href', function() {
expect(el.attr('href')).toBe('/contacts/5');
});

it('should update the href when parameters change', function() {
expect(el.attr('href')).toBe('/contacts/5');
scope.contact.id = 6;
scope.$apply();
expect(el.attr('href')).toBe('/contacts/6');
});

it('should transition states when clicked', inject(function($state, $stateParams, $document, $q) {
expect($state.$current.name).toEqual('');

var e = $document[0].createEvent("MouseEvents");
e.initMouseEvent("click");
el[0].dispatchEvent(e);

$q.flush();
expect($state.current.name).toEqual('contacts.item.detail');
expect($stateParams).toEqual({ id: "5" });
}));
});

describe('forms', function() {
var el, scope;

beforeEach(inject(function($rootScope, $compile) {
el = angular.element('<form ui-sref="contacts.item.detail({ id: contact.id })"></form>');
scope = $rootScope;
scope.contact = { id: 5 };
scope.$apply();

$compile(el)(scope);
scope.$digest();
}));

it('should generate the correct action', function() {
expect(el.attr('action')).toBe('/contacts/5');
});
});
});
11 changes: 10 additions & 1 deletion test/stateSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,16 @@ describe('state', function () {

describe('.href()', function () {
it('aborts on un-navigable states', inject(function ($state) {
expect(function() { $state.href("A"); }).toThrow("State 'A' is not navigable");
expect(function() { $state.href("A"); }).toThrow(
"State 'A' does not have a URL or navigable parent"
);
expect(function() { $state.href("about.sidebar", null, { lossy: false }); }).toThrow(
"State 'about.sidebar' is not navigable"
);
}));

it('generates a parent state URL when lossy is true', inject(function ($state) {
expect($state.href("about.sidebar", null, { lossy: true })).toEqual("/about");
}));

it('generates a URL without parameters', inject(function ($state) {
Expand Down
3 changes: 2 additions & 1 deletion test/test-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ files = [
'src/urlRouter.js',
'src/state.js',
'src/viewDirective.js',
'src/stateDirectives.js',
'src/compat.js',

'test/*Spec.js',
// 'test/compat/matchers.js',
// 'test/compat/*Spec.js',
Expand Down