Skip to content

Removing an inappropriate class used to wrap inline radio form controls. #1239

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
Merged
merged 1 commit into from
Feb 21, 2017
Merged

Removing an inappropriate class used to wrap inline radio form controls. #1239

merged 1 commit into from
Feb 21, 2017

Conversation

sg00dwin
Copy link
Member

@sg00dwin sg00dwin commented Feb 9, 2017

Fixes #1234

Removed the .radio class from two places that use radio-inline controls which in not needed and adds excessive top and bottom margin.

screen shot 2017-02-09 at 3 52 31 pm

screen shot 2017-02-09 at 3 44 53 pm

spadgett
spadgett reviewed Feb 9, 2017
View reviewed changes
app/views/directives/edit-lifecycle-hook.html Outdated
@@ -10,7 +10,7 @@

<div class="form-group">
<label for="actionType" class="required">Lifecycle Action</label><br/>
<div class="radio">
<div>
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the unnecessary div?

@spadgett
Copy link
Member

spadgett commented Feb 9, 2017 •
edited
Loading

+1 to removing class="radio". It looks like that's not meant to be used with radio-inline

edit: based on examples http://getbootstrap.com/css/#checkboxes-and-radios

@sg00dwin sg00dwin force-pushed the radio-btn-spacing-issue1234 branch from 1fb0681 to 3e7d1ed Compare February 10, 2017 15:45
@sg00dwin
Copy link
Member Author

I use the div as a parent wrap for the radio-inline inputs, so that aren't displayed inline with the main .form-group label "Access Mode" and "Lifecycle Action". Also to address the left alignment issue #1240 at mobile we need to remove the bootstrap default left margin on siblings, so < 768 I switched it to the right side, so that they still space apart while inline, but align left when stacked.

screen shot 2017-02-10 at 10 40 21 am

screen shot 2017-02-10 at 10 41 18 am

spadgett
spadgett reviewed Feb 10, 2017
View reviewed changes
app/styles/_forms.less Outdated
.checkbox-inline {
margin-left: 0;
margin-right: 10px;
}
Copy link
Member

@spadgett spadgett Feb 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Any reason not to do this always (instead of max-width xs)? It seems like it could still be a problem with long labels at any width.

Would just the margin styles work without needing the parent div radio-checkbox-group-inline?

.radio-inline,
.checkbox-inline {
  margin-left: 0;
  margin-right: 10px;
}

Copy link
Member

Choose a reason for hiding this comment

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

We only use it on two pages, so should be easy to test the change.

origin-web-console (master) ➜  ag radio-inline
app/views/directives/edit-lifecycle-hook.html
14:          <label class="radio-inline">
22:          <label class="radio-inline">

app/views/directives/osc-persistent-volume-claim.html
95:        <label class="radio-inline">
100:        <label class="radio-inline">
105:        <label class="radio-inline">
origin-web-console (master) ➜  ag checkbox-inline
origin-web-console (master) ➜

@spadgett spadgett self-assigned this Feb 10, 2017
@spadgett spadgett added this to the 1.6.0 milestone Feb 10, 2017
@sg00dwin sg00dwin force-pushed the radio-btn-spacing-issue1234 branch from 3e7d1ed to dd78e13 Compare February 10, 2017 18:02
@sg00dwin
Copy link
Member Author

Ok, updated so that it always applies and it works fine as well.
Needs to include the adjacent rule since that's what bootstrap uses.
https://github.com/openshift/origin-web-console/blob/master/dist/styles/main.css#L708

@spadgett
Copy link
Member

Thanks @sg00dwin , can you fix the indentation since you took out the parent div?

@sg00dwin sg00dwin force-pushed the radio-btn-spacing-issue1234 branch from dd78e13 to e6511c9 Compare February 10, 2017 18:53
@sg00dwin
Copy link
Member Author

Indentation corrected

spadgett
spadgett reviewed Feb 10, 2017
View reviewed changes
app/views/directives/edit-lifecycle-hook.html Outdated
name="{{type}}-action-newpod"
ng-model="action.type"
value="execNewPod"
aria-describedby="action-help">
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm picking nits, but can you indent the attributes as before? I find it harder to read this way.

@sg00dwin
Removing an inappropriate class used to wrap inline radio form controls.
73d21df 
And switch inline margin from left to right side to allow for alignment left.
Fixes openshift#1240
Fixes openshift#1234
@sg00dwin sg00dwin force-pushed the radio-btn-spacing-issue1234 branch from e6511c9 to 73d21df Compare February 10, 2017 19:47
@jwforres
Copy link
Member

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 73d21df

@openshift-bot
Copy link

openshift-bot commented Feb 21, 2017 •
edited
Loading

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1054/) (Base Commit: 9b47dfc)

@openshift-bot openshift-bot merged commit 29a658f into openshift:master Feb 21, 2017
@sg00dwin sg00dwin deleted the radio-btn-spacing-issue1234 branch February 23, 2017 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

Assignees

@spadgett spadgett

Labels
None yet
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sg00dwin @spadgett @jwforres @openshift-bot