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

Router agnostification - POC #253

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rclai
Copy link
Contributor

@rclai rclai commented Jul 27, 2015

This is an untested effort (should still work with Iron Router though) to make this package router agnostic by first making the router dependencies weak, then creating an API Platform.withRouter() that lets you handle router-dependent logic in the components.

I haven't quite parsed the data context to allow for dev-friendly data-context-passing into FlowRouter.path() when creating URLs in certain components but will do this later so that it's easier.

Let me know if anyone actually likes this before I start testing that it works on Flow Router and doing more stuff to it.

Or if anyone wants to try it out, you can always clone the fork branch.

@rclai
Copy link
Contributor Author

rclai commented Jul 30, 2015

@nickw, let me know what you think of this idea and whether I should keep going at it when you have the time 😄

@rclai
Copy link
Contributor Author

rclai commented Aug 4, 2015

Thanks for the feedback guys.


api.use(["templating", "underscore", "fastclick", "tracker", "session"], "client");
api.use('iron:[email protected]', 'client', { weak: true });
api.use('meteorhacks:[email protected] || 2.0.0', 'client', { weak: true });
Copy link

Choose a reason for hiding this comment

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

There is no meteorhacks:[email protected].
It should be kadira:[email protected].

I think we can drop meteorhacks:flow-router at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is going to change, thanks.

@arunoda
Copy link

arunoda commented Aug 5, 2015

How about moving all router specific logic into a separate file. Then if we need to work on a new router, then it's just changing that file.

@rclai
Copy link
Contributor Author

rclai commented Aug 5, 2015

@arunoda, that sounds like a great idea, but I can't envision how to do that since there is router logic in different components. Can you show me a small example?

@arunoda
Copy link

arunoda commented Aug 6, 2015

I mean we can create a set of methods like isActive() and abstract the router related logic. Then, when we need to add a support for a new router, simply change the isActive() code and we don't need to change anything inside the ionic logic related codebase.

@rclai
Copy link
Contributor Author

rclai commented Aug 6, 2015

Ah okay, sounds good. I'll give that a try.

@nickw
Copy link

nickw commented Aug 6, 2015

We'd also need to figure out how to handle rendering into regions:

https://github.com/meteoric/meteor-ionic/blob/master/components/ionNavBar/ionNavBar.html#L3-L5

@dalerka
Copy link

dalerka commented Aug 7, 2015

Apologies if this is not the proper place to mention, but...
Just in case, the option of creating a Router-layer package (to allow using various routers) is discussed here. So, if someone has the necessary skills to build such a package, please take the lead.

@arunoda
Copy link

arunoda commented Aug 7, 2015

@nickw for that we can simply use Meteor's dynamic templates. I think that works for both routers.
Or if that's not possible, we can build two templates. One for IR and one for BlazeLayout (from Flow).

Otherwise we can simply use BlazeLayout in both flow and IR.

@nicolaslopezj
Copy link

I started a router-layer project

https://github.com/nicolaslopezj/meteor-router-layer

@rclai
Copy link
Contributor Author

rclai commented Aug 11, 2015

Oh wow, router-layer might just be the answer. I'll check it out later.

@rclai
Copy link
Contributor Author

rclai commented Aug 17, 2015

@nicolaslopezj, are we able to use the Layout template to render things into regions to tackle the issue that @nickw mentioned for this?

Given that it is {{> yield "regionName"}}, how does that translate in Router Layer.

@nicolaslopezj
Copy link

{{> yield "regionName"}} currently is not interpreted in Router Layer, only {{> yield }} works

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.

5 participants