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

fix: ncyBreadcrumbIgnore flag #99

Merged
merged 4 commits into from
Aug 9, 2015
Merged

Conversation

kazinov
Copy link

@kazinov kazinov commented Aug 6, 2015

Provides the way to ignore some views when rendering breadcrumbs. To achieve this the property ncyBreadcrumbIgnore should be set to true on the view controller scope:

function ViewController($scope) {
       $scope.ncyBreadcrumbIgnore = true;
}

This allows to deal with multiple views related to the same state.
Closes the issue #42 and possibly the issue #62
This plunker demonstrates a common problem with footer view which could be solved with this fix.

@ncuillery
Copy link
Owner

Hi, many thanks for the PR.

The idea of attach a configuration param to the scope sounds a bit dirty to me (I prefer centralize the breadcrumb configuration in the same place) but I recognize it's a simple and effective way to solve two major issues.

It LGTM. I'll make a detailled review of this.

/**
* Module with abstract state with footer view
*/
angular.module('ncy-abstract-interpolation-conf', []).config(function($stateProvider) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think the problem this PR solve is more related to multiple views than abstract view.

Tell me if I'm wrong but it doesn't matter if the state is abstract or not (I remove the abstract: true line in the plunker and the problem still occurs).

I suggest naming the conf 'ncy-multiple-interpolation-conf'here (and update the describe's name below).

@ncuillery
Copy link
Owner

Review done 👍

Please can you amend the commit and changing the title to feat: add the scope-based ncyBreadcrumbIgnore flag and add Closes #42 #62 at the end of the commit message after a blank line.

Thank you for all of this 😄

@kazinov
Copy link
Author

kazinov commented Aug 7, 2015

will do today

kazinov added 2 commits August 7, 2015 17:02
Provide the way to ignore some views when rendering breadcrumbs. To achieve this the property ncyBreadcrumbIgnore should be set to true on the view controller scope.

Closes ncuillery#42 ncuillery#62
Rename ncy-abstract-interpolation-conf to ncy-multiple-interpolation-conf. Make state not abstract. It doesn't matter is there abstract states. The problem is multiple views.
@kazinov
Copy link
Author

kazinov commented Aug 7, 2015

Hi, @ncuillery
I changed the commit name and made some refactoring you asked for.

If you will update the documentation please consider mentioning the special case with inherited scopes.
Example here

ncuillery added a commit that referenced this pull request Aug 9, 2015
feat: add the scope-based ncyBreadcrumbIgnore flag
@ncuillery ncuillery merged commit 4e0d649 into ncuillery:master Aug 9, 2015
@ncuillery
Copy link
Owner

Landed 👍 the release will arrive soon.

I confirm it solves the #62 too !

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.

2 participants