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

fix(radio-group): better alignment for horizontal radios #2375

Closed
wants to merge 10 commits into from

Conversation

@eekbatani
Copy link

eekbatani commented Nov 23, 2019

Fixes #2364

Description

The marginBottom of a radio button component was being set to 'null' in the case that the button had a description with it. This behavior is only correct for vertical alignment, and thus resulting in the distorted output when horizontally aligning the buttons, due to the absence of the bottom margin.

I added a condition to the check that checks if the alignment is not set to horizontal. This way, if the alignment of the radio buttons are horizontal, the bottom margin will not be removed and the buttons will be correctly aligned.
I added Another conditional property for the horizontal alignment, and adds a left margin if it is true. This is to ensure the radio buttons are not stuck together as they were previously. The following screenshot demonstrates the corrected output.

image

Scope

  • Patch: Bug Fix
  • Minor: New Feature
  • Major: Breaking Change
@now now bot temporarily deployed to staging Nov 23, 2019 Inactive
@now

This comment has been minimized.

Copy link

now bot commented Nov 23, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/uber-ui-platform/baseweb/imrh8zfw9
Preview: https://baseweb-git-fork-eekbatani-issue-2364.uber-ui-platform.now.sh

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Nov 23, 2019

CLA assistant check
All committers have signed the CLA.

@gergelyke

This comment has been minimized.

Copy link
Collaborator

gergelyke commented Nov 25, 2019

thank you @eekbatani

@UberOpenSourceBot

This comment has been minimized.

Copy link
Collaborator

UberOpenSourceBot commented Nov 25, 2019

Visual changes were detected on this branch. Please review the following PR containing updated snapshots: eekbatani#1

@@ -137,7 +137,8 @@ export const Root = styled<StylePropsT>('label', props => {
alignItems: 'center',
cursor: $disabled ? 'not-allowed' : 'pointer',
marginTop: sizing.scale200,
marginBottom: $hasDescription ? null : sizing.scale200,
marginLeft: ($align !== 'horizontal') ? sizing.scale200: null,

This comment has been minimized.

Copy link
@chasestarr

chasestarr Nov 25, 2019

Collaborator

Thanks for the contribution. When inspecting the screenshot changes here There should not be margin-left applied when the alignment is vertical. Should the conditional on this line be inverted?

This comment has been minimized.

Copy link
@nadiia

nadiia Nov 25, 2019

Member

Also, the first radio still shouldn't have left margin in horizontal alignment. Could you apply marginRight instead? Could also be :first-child rule but that would rely on no additional wrapper markup.

This comment has been minimized.

Copy link
@eekbatani

eekbatani Nov 26, 2019

Author

You're right, I'll make the changes.

Copy link
Member

nadiia left a comment

Would you mind adding a VRT for the horizontal alignment case?

@@ -137,7 +137,8 @@ export const Root = styled<StylePropsT>('label', props => {
alignItems: 'center',
cursor: $disabled ? 'not-allowed' : 'pointer',
marginTop: sizing.scale200,
marginBottom: $hasDescription ? null : sizing.scale200,
marginLeft: ($align !== 'horizontal') ? sizing.scale200: null,

This comment has been minimized.

Copy link
@nadiia

nadiia Nov 25, 2019

Member

Also, the first radio still shouldn't have left margin in horizontal alignment. Could you apply marginRight instead? Could also be :first-child rule but that would rely on no additional wrapper markup.

@gergelyke

This comment has been minimized.

Copy link
Collaborator

gergelyke commented Nov 26, 2019

@eekbatani would you mind fixing the eslint errors? you can reproduce them locally by running yarn test

@UberOpenSourceBot

This comment has been minimized.

Copy link
Collaborator

UberOpenSourceBot commented Nov 26, 2019

Visual changes have been resolved. eekbatani#1 has been closed. If future commits on issue-2364 trigger visual changes, a new snapshot branch will be created and a new PR will be opened.

@gergelyke gergelyke changed the title fixed distortion bug for horizontal radio groups fix(radio-group): better alignment for horizontal radios Nov 26, 2019
@eekbatani

This comment has been minimized.

Copy link
Author

eekbatani commented Nov 27, 2019

@eekbatani would you mind fixing the eslint errors? you can reproduce them locally by running yarn test

@eekbatani would you mind fixing the eslint errors? you can reproduce them locally by running yarn test

Sure, I didn't see any on my end initially but I can see the issue now. I'll go ahead and fix them.

@gergelyke

This comment has been minimized.

Copy link
Collaborator

gergelyke commented Dec 4, 2019

@eekbatani it seems like eslint is failing - can you look into that please?

@chasestarr

This comment has been minimized.

Copy link
Collaborator

chasestarr commented Dec 4, 2019

@eekbatani it seems like eslint is failing - can you look into that please?

below are the lint errors:

/baseui/src/radio/styled-components.js
--
  | 140:18  error  Replace `($align·===·'horizontal')·?·sizing.scale200` with `$align·===·'horizontal'·?·sizing.scale200·`              prettier/prettier
  | 141:18  error  Replace `·($hasDescription)·&&·($align·!==·'horizontal')·` with `⏎······$hasDescription·&&·$align·!==·'horizontal'`  prettier/prettier

you may want to install the prettier extension into your IDE to auto format the code

@eekbatani

This comment has been minimized.

Copy link
Author

eekbatani commented Dec 4, 2019

$hasDescription·&&·$align·!==·'horizontal'
Sure, I'll make the changes. Also, I did install the prettier extension but I'm assuming something must have gone wrong on my end since it didn't pick up the issue.

@@ -137,7 +137,8 @@ export const Root = styled<StylePropsT>('label', props => {
alignItems: 'center',
cursor: $disabled ? 'not-allowed' : 'pointer',
marginTop: sizing.scale200,
marginBottom: $hasDescription ? null : sizing.scale200,
marginRight: $align === 'horizontal' ? sizing.scale200 : null,

This comment has been minimized.

Copy link
@nadiia

nadiia Dec 5, 2019

Member

Would you mind adding rtl support for it?

@nadiia

This comment has been minimized.

Copy link
Member

nadiia commented Dec 5, 2019

Horizontal positioning in the yard looks missaligned.
Screen Shot 2019-12-05 at 11 23 35 AM

@gergelyke

This comment has been minimized.

Copy link
Collaborator

gergelyke commented Dec 10, 2019

@eekbatani are you planning to working on this pr? happy to take it over if you don't have the time

@eekbatani

This comment has been minimized.

Copy link
Author

eekbatani commented Dec 10, 2019

@eekbatani are you planning to working on this pr? happy to take it over if you don't have the time

Yeah sure. I'm just in my final exam week for school so I'm low on time this week. You can go ahead and take over if you'd like to get it finish it off sooner.

@gergelyke

This comment has been minimized.

Copy link
Collaborator

gergelyke commented Jan 21, 2020

Closing in favor of #2660

@gergelyke gergelyke closed this Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.