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

nateabele · 2013-05-22T22:41:17Z

Hey guys,

So I was messing around and just went ahead and took a pass at implementing a ui-sref directive. Based on my comments from #16 (comment), and elaborating slightly, valid syntax is any of the following:

<a ui-sref="state (params)">...</a>
<a ui-sref="state(params)">...</a>
<a ui-sref="state()">...</a>
<a ui-sref="state">...</a>

...where state and params are any valid parameter pair for $state.transitionTo(), except that within the context of the directive, state is a literal, not a quoted string. The value for params can be an object literal, scope property, or the result of a scope method call. For example:

<li ng-repeat="item in items">
    <a ui-sref="edit({ id: item.id })">{{ item.name }}</a>
</li>

Binding & Events

The directive will auto-update href for links and action for forms. For links, the default click behavior is suppressed in favor of using $state.transitionTo().

Notes

This depends on Method for generating URLs from states #138, which implements $state.href().
I know there was some mention of a more dynamic way of referencing states. IMO that is out-of-scope for a shorthand syntax. If people want something more advanced, they can combine ng-href with $state.href().
I don't have any tests for this yet, as I still need to read up on the proper way to test directives (soon...)

Interested to hear your thoughts, cc: @timkindberg, @ksperling, @jeme, @ProLoser, etc.

ProLoser · 2013-05-22T23:00:26Z

Thanks @nateabele!

timkindberg · 2013-05-22T23:09:44Z

Ill check it out tonight

timkindberg · 2013-05-23T12:56:20Z

Looks great! I'm so glad you decided to just take the initiative on this, I'm sure @ksperling appreciates the help.

Only couple of thoughts:

We should add a couple more tests in describe ".href()" for more route types, i.e. ones with ? type params, and also routes with regex params.
I think we should rename ui-state to ui-stateref. I think it conveys the purpose a bit more clearly. It's like href (hyperlink reference) but for states... stateref... state reference.

timkindberg · 2013-05-23T13:00:38Z

Also just noticed for non-form objects we are adding the associated state url to the href attribute, couple thoughts there:

we then continue to preventDefault(e) anyway, so why is the href attribute needed at all on non-form objects?
also this assumes that a state has a url associated with it, what will happen if no url is defined? Will it throw the "State '" + state + "' is not navigable" error?

nateabele · 2013-05-23T13:20:08Z

We should add a couple more tests in describe ".href()" for more route types, i.e. ones with ? type params, and also routes with regex params.

Yeah, that commit actually got carried over from #138, but, IMO those tests would be kind of redundant. The href() method itself is 3 lines of code. Additional tests for generating URLs would just be duplicates of UrlFactory tests.

I think we should rename ui-state to ui-stateref. I think it conveys the purpose a bit more clearly. It's like href (hyperlink reference) but for states... stateref... state reference.

I don't have a real strong opinion here, but I do love me some terse syntax. :-) Honestly, I don't think it conveys any information that couldn't easily be inferred from context. It might make more sense to me if we had multiple state-related directives. If multiple people feel strongly about this, I can change it.

we then continue to preventDefault(e) anyway, so why is the href attribute needed at all on non-form objects?

Bookmarks... general compatibility with the rest of the web.

also this assumes that a state has a url associated with it, what will happen if no url is defined? Will it throw the "State '" + state + "' is not navigable" error?

Currently it would, yeah. I can add a check to prevent attempts to generate a URL for non-navigable states. I can still see a case where people would want to use links to transition to states that aren't associated with URLs.

jeme · 2013-05-24T09:27:13Z

Is this PR going to need an update after pulling in #138 ??... (I can see that it includes the same change, so I was wondering how Git handles that, still sort of new to Git after all)

nateabele · 2013-05-24T11:46:49Z

@jeme It shouldn't matter.

ksperling · 2013-05-24T11:48:56Z

@nateabele Great work! I'm still not entirely convinced a custom syntax is the way to go, but I guess with scope.eval() doing most of the work it's actually not that much of a problem.

I do think ui-state is somewhat generic as a directive name. What about ui-ref or ui-sref?

Support for form elements is interesting... how would that actually be used?

ksperling · 2013-05-24T11:53:51Z

I think for states without a URL href an option might be to generate a link to the nearest navigable ancestor, but have the onlick transition to the state itself. We would need a flag or options parameter on $state.href() for that -- I think the default behavior of $state.href() should remain to fail on non-navigable.

In terms of tests it might be nice to have a test for testing that changes in the parmeter value are handled, i.e. that href is updated.

nateabele · 2013-05-24T12:33:58Z

I'm still not entirely convinced a custom syntax is the way to go [...]

It was the only way I could think to do a reasonably terse syntax. I'm open to other options.

I do think ui-state is somewhat generic as a directive name. What about ui-ref or ui-sref?

I'm down with ui-ref.

I think for states without a URL href an option might be to generate a link to the nearest navigable ancestor, but have the onlick transition to the state itself [...]

Agreed with all this. I'll try to spend some time on it later today.

nateabele · 2013-05-24T12:47:37Z

Support for form elements is interesting... how would that actually be used?

My initial thought was that it could be used for doing state transitions if the form submit is successful, but that might need to be too tied into how form submits are handled. It would be nice if you could do something like this in an ng-submit handler:

submit: function($event) {
    this.item.$save(function() {
        // Pseudo-code to trigger the transition in the form's ui-ref:
        $event.$success();
    });
}

That way, the controller code would not have to be tightly coupled to state reference information, which logically belongs with the form.

Also, there are generic benefits to generating form actions. For example, in most browsers I can hit Cmd+Enter to target the form submission at a new tab. Obviously this would not work without a URL.

timkindberg · 2013-05-24T13:00:51Z

I like ui-sref better than ui-ref which is too generic still. Needs the 's' to represent state, in my opinion.

ksperling · 2013-05-24T13:02:55Z

@nateabele presumable in the cmd-enter case you'd have to handle the form submit server side? Or are we talking about GET only?

nateabele · 2013-05-24T13:03:25Z

@timkindberg I just decided a few minutes ago that I like it better, too. ;-)

nateabele · 2013-05-24T13:05:04Z

@ksperling Correct, the submit would have to be handled server-side. What? Don't all your apps degrade gracefully, too? ;-)

tamtakoe · 2013-06-16T05:53:52Z

http://plnkr.co/edit/pwACBH?p=preview
Controls work only one time. On my local machine controls didn't work always

nateabele · 2013-06-19T14:07:22Z

After a bit of refactoring, I believe this is ready to roll. It is dependent on #192, which should be merged first. You'll notice the test that verifies the click behavior is a little weird. Unfortunately the methods that are supposed to exist to test events... don't. I have no idea why this is, but I was able to work around it.

Let me know if anybody has any questions.

ksperling · 2013-06-19T22:08:23Z

src/stateDirectives.js

+        if (!nav) return;
+
+        try {
+          element[0][attr] = $state.href(ref.state, params, { lossy: true });


lossy is already the default

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

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.

Okay. I think null would probably be best then.

That works too

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

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

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.

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

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.

ksperling · 2013-06-19T22:48:20Z

just put a few comments in the diff

nateabele · 2013-06-19T22:57:01Z

@ksperling I replied to everything with what I think were adequate explanations. Let me know if you have any follow-ups.

First pass at state directive for auto-generating link URLs

nateabele · 2013-06-23T00:01:41Z

@ksperling Hey, I was gonna change $state.href() to return null instead of throwing an error. Do you still want that done?

ksperling · 2013-06-23T05:06:33Z

Yeah go ahead... I thought I'd just merge this to get the ball rolling.

timkindberg mentioned this pull request Jun 4, 2013

ref attribute #162

Closed

fxck mentioned this pull request Jun 6, 2013

Method for generating URLs from states #138

Merged

First draft of ui-sref directive.

5bcd4cc

ksperling reviewed Jun 19, 2013
View reviewed changes

ksperling added a commit that referenced this pull request Jun 22, 2013

Merge pull request #139 from nateabele/directive

c4d1403

First pass at state directive for auto-generating link URLs

ksperling merged commit c4d1403 into angular-ui:master Jun 22, 2013

nateabele deleted the directive branch June 23, 2013 00:03

nateabele mentioned this pull request Sep 2, 2013

fix(stateDirectives): Generate the proper URL when the state changes #374

Closed

nateabele mentioned this pull request Sep 20, 2013

Dynamic ui-sref in directive #395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nateabele commented May 22, 2013

ProLoser commented May 22, 2013

timkindberg commented May 22, 2013

timkindberg commented May 23, 2013

timkindberg commented May 23, 2013

nateabele commented May 23, 2013

jeme commented May 24, 2013

nateabele commented May 24, 2013

ksperling commented May 24, 2013

ksperling commented May 24, 2013

nateabele commented May 24, 2013

nateabele commented May 24, 2013

timkindberg commented May 24, 2013

ksperling commented May 24, 2013

nateabele commented May 24, 2013

nateabele commented May 24, 2013

tamtakoe commented Jun 16, 2013

nateabele commented Jun 19, 2013

ksperling Jun 19, 2013

nateabele Jun 19, 2013

timkindberg Jun 20, 2013

nateabele Jun 20, 2013

timkindberg Jun 20, 2013

timkindberg Jun 20, 2013

nateabele Jun 20, 2013

laurelnaiad Jun 20, 2013

ksperling Jun 21, 2013

ksperling Jun 21, 2013

ksperling commented Jun 19, 2013

nateabele commented Jun 19, 2013

nateabele commented Jun 23, 2013

ksperling commented Jun 23, 2013

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

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

Conversation

nateabele commented May 22, 2013

Binding & Events

ProLoser commented May 22, 2013

timkindberg commented May 22, 2013

timkindberg commented May 23, 2013

timkindberg commented May 23, 2013

nateabele commented May 23, 2013

jeme commented May 24, 2013

nateabele commented May 24, 2013

ksperling commented May 24, 2013

ksperling commented May 24, 2013

nateabele commented May 24, 2013

nateabele commented May 24, 2013

timkindberg commented May 24, 2013

ksperling commented May 24, 2013

nateabele commented May 24, 2013

nateabele commented May 24, 2013

tamtakoe commented Jun 16, 2013

nateabele commented Jun 19, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksperling commented Jun 19, 2013

nateabele commented Jun 19, 2013

nateabele commented Jun 23, 2013

ksperling commented Jun 23, 2013