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

Added ember-hook to components #41

Merged
merged 10 commits into from
Aug 24, 2016
Merged

Conversation

srowhani
Copy link
Contributor

patch

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f915247 on srowhani:master into * on ciena-frost:master*.

@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Changes Unknown when pulling 57ed6dd on srowhani:master into * on ciena-frost:master*.

@@ -1,6 +1,6 @@
{{#if isVisible}}
{{#if route}}
{{#frost-link route disabled=true}}
{{#frost-link route disabled=true hook=(concat hook '-link')}}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if there are multiple 'route' menu items defined in a nav category? How would one distinguish them via the hook?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b680791 on srowhani:master into * on ciena-frost:master*.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b680791 on srowhani:master into * on ciena-frost:master*.

@rox163
Copy link
Contributor

rox163 commented Aug 24, 2016

README needs an update to show the hooks available. https://github.com/ciena-frost/ember-frost-sort#testing-with-ember-hook

@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Changes Unknown when pulling b7a06e7 on srowhani:master into * on ciena-frost:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b7a06e7 on srowhani:master into * on ciena-frost:master*.

@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Changes Unknown when pulling b7a06e7 on srowhani:master into * on ciena-frost:master*.

| inline action from section context | `$hook('frost-nav-modal-section-<sectionIndex>-action-<actionIndex>')` |
| route hook | `$hook('frost-nav-modal-section-<sectionIndex>-route-<routeIndex>')` |
| frost-link within the route / action context | `$hook('frost-nav-modal-section-<sectionIndex>-(route / action)-<index>-link')` |
| action from section actions context | `$hook('frost-nav-modal-section-actions-action-<index>')` |
Copy link
Contributor

Choose a reason for hiding this comment

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

does the section need an index here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont like this name....'actions-action'. Can we make it a bit shorter?

@rox163
Copy link
Contributor

rox163 commented Aug 24, 2016

👍

Approved with PullApprove

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e589b7e on srowhani:master into * on ciena-frost:master*.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e589b7e on srowhani:master into * on ciena-frost:master*.

@srowhani srowhani merged commit e965c56 into ciena-frost:master Aug 24, 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.

5 participants