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 RadioButton to emitted event #10657

Closed
wants to merge 1 commit into from

Conversation

patrickbussmann
Copy link

This allows to update the checked state manually via the event process.

Short description of what this resolves:

A problem by manually set the checked state of radio buttons.

The main problem is that I want to uncheck a radio button within a radio-group if its clicked and not required. But currently it stays checked. I want if its not required that its unchecked then like a checkbox. But then I lose the ability to check one radio button at the same time.

Changes proposed in this pull request:

  • Added second parameter to event emitter of radio button with this value (first parameter is this.value)

Ionic Version: 1.x / 2.x
2.x

Fixes: #
-/-

This allows to update the checked state manually via the event process.
@brandyscarney
Copy link
Member

I'm sorry I'm not fully understanding what you're trying to do. Could you provide a plunker, repository or images of what you mean? Thanks!

@patrickbussmann
Copy link
Author

patrickbussmann commented Mar 16, 2017 via email

@manucorporat
Copy link
Contributor

manucorporat commented Mar 18, 2017

@brandyscarney I think this is another case of: #8578

some components do:

    this.ionSelect.emit(this.value);

while others do:

    this.ionSelect.emit(this);

Changing this would be an important breaking change, but I think it worth it... we should always provide this, it is way more powerful.

Then, developers could read the values thought the value property.

@manucorporat
Copy link
Contributor

manucorporat commented Mar 18, 2017

I don't think we should merge this PR, but we must do something about it.

@patrickbussmann
Copy link
Author

Hmm ok. I added this as second parameter to be compatible with previous versions 👍
But if you have other ideas tell us them.

@brandyscarney
Copy link
Member

I was thinking the same thing @manucorporat. I've noticed before that we don't always emit this. Need to discuss with @adamdbradley to see if he had a reason for it, but otherwise I'm all for it.

@Ionitron
Copy link
Collaborator

Hello and thank you for contributing to Ionic! We have been working on porting all of the Ionic components to web components and have recently updated master to reflect this. This significant change has caused this pull request to break. While we really appreciate the time and effort you put into creating this, we are not able to merge it because of the newly introduced conflicts. We are extremely sorry about this. We will not be merging any more features in to v3. If this is a feature and you have the time, please resubmit this PR against the master branch. If this is a critical security issue in v3, we would greatly appreciate it if you would resubmit the PR against the new v3 branch. Thanks so much for your time!

@Ionitron Ionitron closed this Mar 12, 2018
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.

4 participants