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

feat(uiSrefActive): nested state and DOM decendant support for ui-sref-active #821

Closed
wants to merge 1 commit into from

Conversation

MrOrz
Copy link

@MrOrz MrOrz commented Jan 25, 2014

uiSrefActive now takes all decendant uiSrefs into consideration.
uiSrefActive also adds a class with -nested postfix when a child state of the related uiSrefs becomes active.

Implemented the consensus we previously met in #704.

…f-active

uiSrefActive now takes all decendant uiSrefs into consideration.
uiSrefActive also adds a class with "-nested" postfix when a child state of the related uiSrefs becomes active.

Closes angular-ui#704.
@MrOrz
Copy link
Author

MrOrz commented Jan 26, 2014

Not sure if the $timeout hack should be replaced with $evalAsync instead.

http://stackoverflow.com/questions/17301572/angularjs-evalasync-vs-timeout

@nateabele
Copy link
Contributor

Not a huge fan of this implementation. It seems like it:

  • Contains a lot more code than it should to solve the original problem statement
  • Contains 'magical' classes
  • Uses events where you wouldn't need any if you had a good directive controller design.

Not sure if the $timeout hack should be replaced with $evalAsync instead.

Nope.

@timkindberg
Copy link
Contributor

I am a fan of the what is trying to be achieved though, more so than #819.

@nateabele
Copy link
Contributor

[M]ore so than #819.

Two completely different problems.

@timkindberg
Copy link
Contributor

It is? I gathered that they were the same.

From the PR description above...

uiSrefActive also adds a class with -nested postfix when a child state of the related uiSrefs becomes active

From #819...

This issue provides support for optionally counting a link as active if any of its child states are active and parameters match.

I thought they were both solving the same problem—activate a ui-sref link when its child state becomes active—but in two different ways. #819 just activates it, whereas this PR gives it a different active class, e.g. '-nested' postfix.

@MrOrz
Copy link
Author

MrOrz commented Jan 27, 2014

Contains a lot more code than it should to solve the original problem statement
Contains 'magical' classes
Uses events where you wouldn't need any if you had a good directive controller design.

In #704 @besnik81 provided his implementation on the same problem, which does not have the three problems @nateabele addressed above.

Here is @besnik81's implementation : https://github.com/besnik81/ui-router/compare/patch-2

It does the following:

  • Provides the ability to nest uiSrefActive and is able to support nested state with a few modification on one single function. (More specifically, the if-statement in checkNotifySrefActive)
  • Supports ui-sref-active="is-active|is-parent-active" syntax.
  • Recursively traverse the directive controller upwards. No events used.

If my implementation is not good enough, please take a look at @besnik81 's. It's a solution to both #818 (targeting nested-states) and #704 (mentioning DOM hierarchy as well) .

@tomekpiotrowski
Copy link

What is the current status of:

uiSrefActive now takes all decendant uiSrefs into consideration.

?

From what I can see it is still not implemented in master - only the last child uiSref is considered. I can provide a PR that implements this functionality on top of current master if needed.

@timkindberg
Copy link
Contributor

@tomekpiotrowski oh you may be right. Sure submit a PR if you like.

@ProLoser
Copy link
Member

Thoughts on #818 #819 and any other related ones:

ui-is-sref="active-class" for exact matches and ui-has-sref="active-class" for loose matches. Frankly if you want to make this whole thing any more complicated you might as well point people to just do ng-class="...$state.contains(...)"

@timkindberg
Copy link
Contributor

We already have ui-sref-active for fuzzy matches and ui-sref-active-eq for exact.

@stale
Copy link

stale bot commented Jan 24, 2020

This issue has been automatically marked as stale because it has not had
recent activity. It will be closed if no further activity occurs.

This does not mean that the issue is invalid. Valid issues
may be reopened.

Thank you for your contributions.

@stale stale bot added the stale label Jan 24, 2020
@stale stale bot closed this Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants