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

Adds name option to groups #290

Merged
merged 5 commits into from
Aug 27, 2015
Merged

Adds name option to groups #290

merged 5 commits into from
Aug 27, 2015

Conversation

richsilv
Copy link
Contributor

Sometimes it's useful to know which group the current route belongs to (for example, is it only for logged-in users?). Currently, that's not possible unless you're using prefixes for that group.

Allowing the user to pass a name option resolves this as it allows you to use FlowRouter.current().route.group.name to identify the current group.

@@ -1,7 +1,7 @@
Package.describe({
name: 'kadira:flow-router',
summary: 'Carefully Designed Client Side Router for Meteor',
version: '2.3.0',
version: '2.3.0_1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't increment this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, that was just so I could tell I was using the new version locally.

@arunoda
Copy link
Contributor

arunoda commented Aug 26, 2015

One more thing?
Is it possible to have a test case for this?

On 2015 අගෝ 26, බදාදා at ප.ව. 6.30 Richard Silverton <
notifications@github.com> wrote:

Sometimes it's useful to know which group the current route belongs to
(for example, is it only for logged-in users?). Currently, that's not
possible unless you're using prefixes for that group.

Allowing the user to pass a name option resolves this as it allows you to

use FlowRouter.current().route.group.name to identify the current group.

You can view, comment on, or merge this pull request online at:

#290
Commit Summary

  • Adds support for group names
  • removed browserify files

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#290.

@richsilv
Copy link
Contributor Author

👍

@arunoda
Copy link
Contributor

arunoda commented Aug 27, 2015

Awesome.

arunoda added a commit that referenced this pull request Aug 27, 2015
Adds `name` option to groups
@arunoda arunoda merged commit d504b2b into kadirahq:master Aug 27, 2015
@serkandurusoy
Copy link

I have two quick questions:

  1. What happens if groups are nested? Which name would the method return?
  2. Does it not make sense to create getGroupName() that depends on _routeClosed so that we can get the name reactively?

@richsilv
Copy link
Contributor Author

@serkandurusoy

  1. name is just a property rather than a method, so you can get the deepest-nested group name with FlowRouter.current().route.group.name, and its parent group's name with FlowRouter.current().route.group.parent.name, and so on.
  2. If you want to do something reactive with name, you just need to put FlowRouter.watchPathChange() in your reactive function, and the computation will rerun whenever the current group name changes (or its parent group, etc.).

@serkandurusoy
Copy link

Thanks this has been really helpful, especially with an "active route group" helper I can now use in a deeply nested menu and route structure. 👏

@arunoda
Copy link
Contributor

arunoda commented Aug 28, 2015

Btw: Shall we add this into the README?
Could you send me a PR to the group section with how to access this.
On 2015 අගෝ 28, සිකු at ප.ව. 3.31 Serkan Durusoy notifications@github.com
wrote:

Thanks this has been really helpful, especially with an "active route
group" helper I can now use in a deeply nested menu and route structure. [image:
👏]


Reply to this email directly or view it on GitHub
#290 (comment).

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.

3 participants