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 ionDragEnd output event to be emitted on leave for ion-range #10761

Merged
merged 3 commits into from
Mar 15, 2017

Conversation

AmitMY
Copy link
Contributor

@AmitMY AmitMY commented Mar 13, 2017

Short description of what this resolves:

Currently, there is no way to know if ionic-range is being focused or not.

Changes proposed in this pull request:

  • Add an output event called ionDragEnd to ion-range, that is being triggered when the range is being left (pointer up)
  • <ion-range (ionDragEnd)="onDragEnd($event)"></ion-range>

Ionic Version: 2.2.0

Fixes: This feature request: #10760

@manucorporat
Copy link
Contributor

Maybe ionDragEnd better?

@AmitMY AmitMY changed the title Added ionLeave output event to be emitted on leave for ion-range Added ionDragEnd output event to be emitted on leave for ion-range Mar 13, 2017
@AmitMY
Copy link
Contributor Author

AmitMY commented Mar 13, 2017

@manucorporat Thanks, that is indeed a better name. Changed

@manucorporat
Copy link
Contributor

@AmitMY thanks! btw I can change the commit title, but please review the commit message style next time you submit a PR.
https://github.com/driftyco/ionic/blob/master/.github/CONTRIBUTING.md#commit-message-format

@AmitMY
Copy link
Contributor Author

AmitMY commented Mar 15, 2017

@manucorporat My apologies. Definitely will do next time.
Is this PR in a ready-to-merge state, or is there anything missing?

@manucorporat
Copy link
Contributor

manucorporat commented Mar 15, 2017

@AmitMY you are going to kill me hahah
so I have been talking with the team, and we think we should use ionBlur and ionFocus just like we use with ion-input

Eventually we should implement this events in all the ionic inputs consistently: #8578

Can you modify the PR again using ionBlur? would you mind trying to implement ionFocus as well?

ionFocus = ionDragStart

We don't like ionDragEnd because it is not agnostic and there is not ionDrag event, but ionChange.

Thanks again for your time!

@AmitMY
Copy link
Contributor Author

AmitMY commented Mar 15, 2017

I'll be happy to change even 10 times if this gets merged :)

About dragStart - you supplied all of the methods:

pointerDown: this._pointerDown.bind(this),
pointerMove: this._pointerMove.bind(this),
pointerUp: this._pointerUp.bind(this),

Its just a matter of emitting blur and focus. Done

Any way this can be in 2.3 or no way?

Edit:
sorry. just now realised I ignored the commit style guide (again).

@manucorporat
Copy link
Contributor

sure, I expect to merge this today if all the requested changes are added!!

@@ -360,6 +365,9 @@ export class Range extends Ion implements AfterViewInit, ControlValueAccessor, O
return false;
}

// trigger ionFocus event
this.ionFocus.emit(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this at the bottom of the function just to make sure we don't break anything!

Copy link
Contributor Author

@AmitMY AmitMY Mar 15, 2017

Choose a reason for hiding this comment

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

I think it would be more correct there, as if I put this at the end, the order of firing would be:
ionChange -> ionFocus -> ionChange x many -> ionBlur

Instead of:
ionFocus -> ionChange x (many+1) -> ionBlur

Still think I should change?

Copy link
Contributor

Choose a reason for hiding this comment

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

shit! you right

@manucorporat
Copy link
Contributor

manucorporat commented Mar 15, 2017

@AmitMY don't worry about the commit style guide in this PR!

@manucorporat manucorporat merged commit 8f310eb into ionic-team:master Mar 15, 2017
@manucorporat manucorporat added this to the 2.3.0 milestone Mar 15, 2017
@manucorporat
Copy link
Contributor

@AmitMY merged! thanks a lot for the PR!

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