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

Add RadioButton components #8009

Merged
merged 5 commits into from
Mar 8, 2022
Merged

Add RadioButton components #8009

merged 5 commits into from
Mar 8, 2022

Conversation

nkuoch
Copy link
Contributor

@nkuoch nkuoch commented Mar 4, 2022

Fixed Issues

Needed by #7813 (comment)

Tests

Tested in #7813 (comment)

PR Review Checklist

None

Screenshots

Web

image

@nkuoch nkuoch requested a review from a team as a code owner March 4, 2022 11:31
@nkuoch nkuoch requested review from marcaaron and removed request for a team March 4, 2022 11:32
@MelvinBot MelvinBot requested a review from timszot March 4, 2022 11:32
@nkuoch nkuoch requested review from shawnborton and removed request for timszot March 4, 2022 11:32
@nkuoch nkuoch self-assigned this Mar 4, 2022
@nkuoch nkuoch changed the title Add RaddioButton components Add RadioButton components Mar 4, 2022
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

Nice, but something looks off about RadioButtons


render() {
return (
<view>
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing import for this and also incorrect case. Surprise this did not error 🤔

@shawnborton
Copy link
Contributor

Looks pretty good, but maybe we want just a little bit less space between the radio button and the text? I think we want something like 12px between them.

@nkuoch
Copy link
Contributor Author

nkuoch commented Mar 5, 2022

Ok, changing the margin for checkbox labels too then.

Before:
image

After:
image

@nkuoch
Copy link
Contributor Author

nkuoch commented Mar 5, 2022

Updated!

borderRadius: 10,
height: 20,
width: 20,
borderColor: themeColors.border,
Copy link
Contributor

Choose a reason for hiding this comment

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

We've gotten feedback before that the border color for these feels way too light. Maybe that's a separate problem, but I could see bumping up the gray one step to using themeColors.icon which would make it a bit darker. Could I see a before/after on that so we can take a look? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before:

image

After:

image

@nkuoch
Copy link
Contributor Author

nkuoch commented Mar 8, 2022

Updated!

@marcaaron marcaaron self-requested a review March 8, 2022 19:30
@marcaaron marcaaron merged commit 0de8590 into main Mar 8, 2022
@marcaaron marcaaron deleted the nat-radiobutton branch March 8, 2022 19:31
@OSBotify
Copy link
Contributor

OSBotify commented Mar 8, 2022

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcaaron in version: 1.1.42-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@mananjadhav
Copy link
Collaborator

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issue in the future.

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.

5 participants