dotJEM / angular-routing

Enhanced state based routing for Angular applications!
https://dotjem.github.io/angular-routing/
MIT License
75 stars 9 forks source link

Sticky views and resolve #49

Closed soundstep closed 11 years ago

soundstep commented 11 years ago

Hi,

I'm trying out your module which looks great, after having an issue with the ui.router one. My goal is not reload the template html, see the issue: https://github.com/angular-ui/ui-router/issues/364

The sticky views seem to be exactly what I need. Here is what I use:

.state('dashboard', {
    route: '/dashboard/:id',
    views: {
        'main': {
            sticky: true,
            template: 'dashboard.html',
            controller: 'DashboardController'
        }
    },
    resolve: {
        dashboardData: ...
    }
})

From a route: /dashboard/123 To: /dashboard/456

When the route change occurs, I can see that a refresh function is called on the controller:

$scope.refresh = function(data) {
    console.log('refresh', data); // data is {sticky: true}
};

But the data from the resolve property doesn't seem to be sent to the refresh function, I receive instead an object with a sticky property set to true ({sticky: true}), is that an issue?

I also noticed I can use $view.setOrUpdate, which I tried in the controller itself. I do receive the data in the fourth parameter.

$view.setOrUpdate = function(viewname, template, controller, locals, sticky) {
    console.log('SET OR UPDATE', locals); // locals contains the data
};

The setOrUpdate also receives the controller, but I used it in the controller, where is the $view suppose to be used?

Lastly, there's a typo there: https://github.com/dotJEM/angular-routing/blob/master/src/view.ts#L353

$view.refesh(key, data);

should be (missing the R):

$view.refresh(key, data);

I can create a pull request if you do not wish to solve that one directly.

i'd love to get some lights on all this, thanks for any help!

Romu

jeme commented 11 years ago

Thanks for the feedback.

I can understand you a bit confused out on the parameters passed in the refresh method, especially as the documentation on that part is properly lacking. The idea with that parameter was actually not to provide the parameters as you expected, but more to be able to provide information about origin etc.

This is because you are able to call the "refresh" your self in a completely different context where parameters would be something else.

I am currently working on making parameters more accessible in different ways, and I think it makes sense to look at the refresh as part of that and maybe add the parameters to the object passed in refresh, so they are also accessible there, however they will have to be a "child" property due to the ability mentioned above. So something like "$params" on that object. (the $ to avoid conflicts)

But for now I guess your options are:

Which is not all that great as both are subject for change in the near future... The later is the "one" that was meant to be used, but I realized that it was horrific to use due to all the .'s you had to type, so I am looking for ways to expose the parameters more directly which will be in the next minor release (4.1).

And also thanks for spotting the Typo, there are properly more of those and the TypeScript files are certainly not up to date either. :S...

Pull requests will be nice, but if you just wish to wait till ill take care of it that will be ok as well... It will certainly be a fast fix so Ill prioritize it for the next release as well... But create a separate issue.

soundstep commented 11 years ago

Thanks for the response!

And sorry it was unclear, but what I was trying to get is not the params but the data pulled from the resolve property, "dashboardData" in my example.

Which I got from $view.setOrUpdate inside the controller, but I was just wondering why I would not get that in the refresh function.

Hope that makes sense!

soundstep commented 11 years ago

I had a try to get the data back in the refresh, what you call "locals", I change that line:

https://github.com/dotJEM/angular-routing/blob/master/src/view.ts#L207

To:

raiseRefresh(name, {
    template: template,
    locals: locals,
    controller: controller,
    sticky: sticky
 });

I might fork and work on it at some point, because this router goes where it should, especially with the transitions, just taken by the time right now...

What do you think about that change?

soundstep commented 11 years ago

Comment removed, issue created: https://github.com/dotJEM/angular-routing/issues/52

jeme commented 11 years ago

Arh... ok... haven't quite thought of resolve in this context and didn't catch that you referred to that...

There is properly some issues with the change you propose as it might give a wrong idea of what "controller" is at that point as controller is at that point, (it's the function and not the instantiated controller)...

So I guess I would have a question to what you would achieve by putting on the template and controller?...

Putting the locals on there shouldn't cause any issues... Need to figure out how to deal with that particular in cases where you raise the "$view.refresh" else where... If there is any meaningful way to provide the locals in that case as well, in that case it would probably be { $locals: locals, sticky: sticky }...

jeme commented 11 years ago

Yes please raise that as a different issue so they can be tracked separately :)... That is also more a straight up bug... (something will actually not work the way it is suppose to)

but and that one would get put on for 0.4.1 where I have scheduled this for 0.4.2 for now...

soundstep commented 11 years ago

I actually don't need the controller and the template, only the locals, this works for me:

raiseRefresh(name, {
    locals: locals,
    sticky: sticky
});
jeme commented 11 years ago

For now it's a "reserved" property (starts with $, so $locals)... and locals are also added in the refresh... And there should be tests to verify this. Ill still keep this open for a little while longer to hear if it solved your issue.

soundstep commented 11 years ago

I just got the latest version, I get the $locals now. Thanks and sorry again not creating a pull request, I'm not making that easy for you ;)

jeme commented 11 years ago

Your feedback is more than welcome, no reason to apologize for that, no contribution are to small... As long as people don't get demanding :).

soundstep commented 11 years ago

Absolutely! I tend to find the solutions on my own anyway, open source is a collaboration thing. But it could have been neater with a fork and a pull request from my side ;)

I'm using it in production, there's nothing as advanced as this one apparently, but regardless of its flag (still kind of beta right?), I don't have time to build something custom.

So, thanks anyway for the fast answers!

jeme commented 11 years ago

I'm using it in production, there's nothing as advanced as this one apparently, but regardless of its flag (still kind of beta right?), I don't have time to build something custom.

That is correct, the core is quite stable and I have done as far as time allowed for writing through tests for most areas, but there are still areas and components that could use better testing...

But beyond that, I have kept it in pre-release / beta more because of the API and that there might still be breaking changes...

But the thing that really lowers confidence in the module is the fact that it's not widely used yet... I use it in a private project and we have started using it for a project at work, but none of those two are in production and they also don't utilize all the corners of the framework...

So it's only great to get more people out there to use it so it can be hardened... So by all means, feel free to ask...