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 ionModalView #1668

Closed
wants to merge 1 commit into from
Closed

Conversation

joshdholtz
Copy link

Makes adding a modal a little more Ionic Framework-y - Closes #1667

Anything thoughts, comments, or critiques are welcome - this is my first JS contribution to the world :)

OLD WAY

<div class="modal">
  <ion-content>
    Josh is a pretty cool guy
  </ion-content>
</div>

NEW WAY

<ion-modal-view>
  <ion-content>
    Josh is a pretty cool guy
  </ion-content>
</ion-modal-view>

OR OTHER NEW WAY

<div ion-modal-view>
  <ion-content>
    Josh is a pretty cool guy
  </ion-content>
</div>

@perrygovier
Copy link
Contributor

I'm on the fence about this. While it's a solution to #1667, I'm not sure it's the "proper" solution to that issue.

With that, while it does feel more "ionic-ey", I'm not sure what the benefit is. I'd like to add this, but I'd want to see more baked in functionality.

@ajoslin, maybe this could be extended to interact with the $ionicModal service in a similar way to how we discussed the popover approach.

@ajoslin
Copy link
Contributor

ajoslin commented Jul 7, 2014

This is separate from #1667. This is just making it so we don't have to write <div class="modal">.

I like it.

@ajoslin ajoslin closed this in ed4f228 Jul 7, 2014
@joshdholtz
Copy link
Author

@ajoslin Quick question for you. Is there a reason this PR wasn't merged and my code was duplicated (without the tests) in that commit? Just curious on what the policy for Ionic pull requests are. Thanks!

@ajoslin
Copy link
Contributor

ajoslin commented Jul 9, 2014

My bad Josh ... I got carried away with doing other modal features and additions at the same time and forgot to pull your version down.

@ajoslin
Copy link
Contributor

ajoslin commented Jul 9, 2014

The norm is for us to merge other code. If it's right, I try as much as possible to pull down contributors' code, change a couple of lines if needed, and merge theirs.

If you look in the commit log, you can see three PRs that were merged Monday. https://github.com/driftyco/ionic/commits/master

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.

ion-view pushes to stack
3 participants