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.

Sign up for GitHub

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

Jump to bottom

feat(RadioButtons): add RadioButtons #294

Merged
merged 1 commit into from
Oct 28, 2021
Merged

feat(RadioButtons): add RadioButtons #294

merged 1 commit into from
Oct 28, 2021

Conversation

agreatscott
Copy link
Contributor

@agreatscott agreatscott commented Oct 28, 2021 •
edited
Loading

close #293

The important change is removing that onEffect, since we want the options to be dynamic, so if the user performs an action elsewhere on the page or an asyc event completes that changes the options, we reflect that change.

Maybe this is a breaking change though? cc @akdetrick

akdetrick
akdetrick reviewed Oct 28, 2021
View reviewed changes
src/RadioButtons/index.stories.js
@@ -0,0 +1,42 @@
import React, { useState } from "react";
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙏 thank you for adding a story

akdetrick
akdetrick reviewed Oct 28, 2021
View reviewed changes
src/RadioButtons/index.stories.js
Green: "green",
Yellow: "yellow",
}}
name="colours"
Copy link
Collaborator

Choose a reason for hiding this comment

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

🇬🇧

@akdetrick
Copy link
Collaborator

This is a little ambiguous - you're right that this would normally be a breaking change, but I would recommend instead you reword your commit as a feat instead of a fix. This PR essentially adds RadioButtons to the documented, publicly available components.

We have a lot of dead components in our tree right now... The Table component for example, still uses styled-components and has never been documented in storybook or used by any external application.

To deal with this weird state of NDS, I am considering components "Available" only if they meet one or both of these conditions:

  • The component is already used in banking
  • The component is documented in storybook

tl;dr I consider this change a feat because it exposes RadioButtons in documentation for the first time.

akdetrick
akdetrick requested changes Oct 28, 2021
View reviewed changes
Copy link
Collaborator

@akdetrick akdetrick left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM - before merging, can we reword the commit as something like feat(RadioButtons): Add RadioButtons instead of fix...?

@agreatscott agreatscott changed the title fix(RadioButtons): allow for options to be updated via change of the options prop feat(RadioButtons): add RadioButtons Oct 28, 2021
@agreatscott agreatscott merged commit 9e77354 into master Oct 28, 2021
akdetrick pushed a commit that referenced this pull request Oct 28, 2021
@semantic-release-bot
chore(release): 1.6.0 [skip ci]
2935b55 
## [1.6.0](v1.5.0...v1.6.0) (2021-10-28)

### Features

* **RadioButtons:** add RadioButtons ([#294](#294)) ([9e77354](9e77354))
@akdetrick
Copy link
Collaborator

🎉 This PR is included in version 1.6.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
Reviewers

@akdetrick akdetrick akdetrick approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RadioButtons] options cannot be changed after initial render
2 participants
@agreatscott @akdetrick