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

[RadioGroup] Add tests for edge cases #6409

Merged
merged 2 commits into from Mar 25, 2017

Conversation

agamrafaeli
Copy link
Contributor

@agamrafaeli agamrafaeli commented Mar 21, 2017

  • Focus with no radios

  • Internal radios should fire onChange

  • on mount register internal radios to this.radios

  • PR has tests / docs demo, and is linted.

  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".

  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

- Focus with no radios
- Internal radios should fire onChange
- on mount register internal radios to this.radios
@muibot muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 21, 2017
@agamrafaeli
Copy link
Contributor Author

I'm not sure this still needs revision?

@mbrookes
Copy link
Member

@agamrafaeli Absolutely. "Needs revision" isn't removed automatically, as there may be other reasons "Needs revision" was added that aren't addressed by the most recent push.

@mbrookes mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Mar 22, 2017
before(() => {
mount = createMount();
});

Copy link
Member

Choose a reason for hiding this comment

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

We miss the teardown.

  after(() => {
    mount.cleanUp();
  });

@oliviertassinari oliviertassinari added next PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI test and removed PR: Needs Review labels Mar 22, 2017
@oliviertassinari
Copy link
Member

I'm gonna fix the issue once merged. +0.4% @agamrafaeli Thanks.

@oliviertassinari oliviertassinari merged commit f1ca1a9 into mui:next Mar 25, 2017
@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 25, 2017
@agamrafaeli agamrafaeli deleted the radio-group branch April 1, 2017 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants