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

[BTS-165] Radio button group component #39

Merged
merged 14 commits into from
Apr 5, 2019
Merged

Conversation

ddsilva
Copy link
Contributor

@ddsilva ddsilva commented Apr 3, 2019

radio-button-group

Copy link
Contributor

@rapahaeru rapahaeru left a comment

Choose a reason for hiding this comment

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

stories/RadioGroup/sub-components/buttonGroup.jsx Outdated Show resolved Hide resolved
components/shared/HiddenRadio.jsx Show resolved Hide resolved
components/Button/buttonStyle.js Outdated Show resolved Hide resolved
components/RadioGroup/RadioButton.jsx Show resolved Hide resolved
components/RadioGroup/RadioButton.jsx Outdated Show resolved Hide resolved
components/RadioGroup/RadioButton.jsx Outdated Show resolved Hide resolved
@ddsilva ddsilva requested a review from rapahaeru April 5, 2019 14:51
Copy link
Contributor

@alan-oliv alan-oliv left a comment

Choose a reason for hiding this comment

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

I Left two comments where I suggest to use a generator function on uniqId module...
Since the uniqId module is not on this PR, we would've to refactor it to look something like this:

export default function* (prefix = '') {
  let _counter = 0;
  while(true)
    yield `${prefix}${_counter++}`;
}

components/RadioGroup/RadioButton.jsx Outdated Show resolved Hide resolved
components/RadioGroup/RadioButton.jsx Show resolved Hide resolved
@ddsilva ddsilva merged commit fa98f99 into master Apr 5, 2019
@delete-merged-branch delete-merged-branch bot deleted the BTS-165-radio-button-group branch April 5, 2019 19:13
@catho-machine-user
Copy link

🎉 This PR is included in version 1.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

5 participants