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(ncyBreadcrumbLast): add custom templating #133

Merged

Conversation

freethejazz
Copy link
Contributor

Add ability to set a custom template or templateUrl for
ncyBreadCrumbLast. Before, if a custom template that allowed HTML via
ngBindHtml, breadcrumb last would not behave properly since it escaped
HTML by default. A value could be assigned to the ncyBreadcrumbLast
directive to customize the template, but it would have to be done on
each instance of the directive used. Now, a template can be set once via
the templateLast and templateLastUrl properties of the configuration
object passed to $breadcrumbProvider.setOptions.

A few things to point out:

  1. ncyBreadcrumb replaces the element the directive is set to, but ncyBreadcrumbLast does not. This means that the custom template that a user sets for ncyBreadcrumbLast will be wrapped by whatever element they added the directive to. This shouldn't be a huge problem, as it's the expected behavior today when assigning a custom value to ncyBreadcrumbLast.
  2. I cringed a bit at just copying the two existing methods for getTemplateLast and getTemplateLastUrl, but I think it's probably the best way to go. I thought the added complexity of refactoring those functions for reusability was not worth it, considering they probably won't be used anywhere else. If you want those to be DRYer, let me know and I can do that.
  3. I'm not married to any of the identifier names introduced here, so if any of them are displeasing let me know of your desired alternative and I'll change it.
  4. In order to pass all the tests, I had to lock the testLatest version of angular to 1.4.9, instead of using the actual latest of 1.5.0. This is due to the fact that angular 1.5.0 causes tests to fail in master.
  5. This functionality should be documented once it makes it's way into master.

Closes #123, Closes #125

@freethejazz
Copy link
Contributor Author

Regarding # 4 above, I did not commit the version lock to the Gruntfile, so integration tests will likely fail on the testLatest run.

@freethejazz
Copy link
Contributor Author

I've opened a new issue (#135) to address the problem in the above comment

@ncuillery
Copy link
Owner

Thanks for this well-tested / well-explained PR ! I sounds good to me.

We'll fix CI soon and then merge this one.

@ncuillery
Copy link
Owner

Hi, can you rebase your PR please ? Tests should be working.

Add ability to set a custom template or templateUrl for
ncyBreadCrumbLast. Before, if a custom template that allowed HTML via
ngBindHtml, breadcrumb last would not behave properly since it escaped
HTML by default. A value could be assigned to the ncyBreadcrumbLast
directive to customize the template, but it would have to be done on
each instance of the directive used. Now, a template can be set once via
the templateLast and templateLastUrl properties of the configuration
object passed to $breadcrumbProvider.setOptions.
@freethejazz freethejazz force-pushed the feat/custom-breadcrumb-last-template branch from 22619b4 to 974f99b Compare April 10, 2016 22:00
@freethejazz
Copy link
Contributor Author

Rebased and pushed! Tests worked locally, so I think we might be in the clear. (Let's see what travis has to say about it)

@ncuillery
Copy link
Owner

Landed, thanks !

I wait few days if recent PR are rebased by their authors, then I release a new version (will be the 0.5.0)

@ncuillery ncuillery closed this Apr 11, 2016
@ncuillery ncuillery reopened this Apr 11, 2016
@ncuillery ncuillery merged commit 2e7eaaf into ncuillery:master Apr 11, 2016
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.

None yet

2 participants