Forms checkbox radio update

[nastiatikk]

Status: Ready for Review Owner: Anastasia Reviewers: @ry5n @avelinet @mlworks @cole-sanderson @kpeatt @jeffkamo

Changes

• index.html markup changed to match _forms.dust markup
• Checkbox and radio styles in forms.scss changed to match _forms.dust markup
• Long labels performance tweaked and they properly drop to the new line

  Todos:

• [x] Designer +1 (@jeffkamo)
• [x] Designer +1 @ry5n

[ry5n]

@nastiatikk Can you expand on the issue summary? I can’t tell why these changes are being made – why does the dust sample markup not match what’s in the tests? How do we know which is correct?

Also, the re-written index.html in test/ uses tabs instead of spaces and seems to have indent issues in general.

[nastiatikk]

@ry5n We have discrepancy between _forms.dust, _forms.scss and index.html files.

If you check /test/index.html mark up you will see this:

<label>
    <input type="radio" class="radio" name="radio_button" value="radio_1"> 
    Radio 1
</label>

If you check /templates/_forms.dust you will see this:

<div>
    <input id="radio-button" type="radio" />
    <label for="radio-button">A radio button and its label!</label>
</div>

forms.dust has markup that we usually use in our projects, but Vellum scss didn't match that markup. I made them all match and also made a tweak that correctly drops label to the new line if it's too long.

PS: index.html had 2 spaces tabs, I changed it to 4 spaces but forgot to switch from tabs.

[nastiatikk]

From the files history index.html wasn't updated since it was created on Mar 31, 2014 whilst forms.dust was updated on Nov 19, 2014

[nastiatikk]

Forgot to mention that with forms.dust file markup we can style disabled states of radio and checkbox, what we can't do with index.html markup

[jeffkamo]

Perhaps the scope of this PR should focus on cleaning up the markup. The changes to the styles is something I'm not sure we want would want to do here.

[ry5n]

Yes, let’s focus this PR on moving all example markup into one place and deciding how it should look.

[nastiatikk]

So, we need to decide then what is our default markup:

№1
<label>
    <input type="radio" class="radio" name="radio_button" value="radio_1"> 
    Radio 1
</label>

OR

№2
<div>
    <input id="radio-button" type="radio" />
    <label for="radio-button">A radio button and its label!</label>
</div>

And anyway change the styles because №1 is not that we use in our projects and №2 doesn't match our Vellum styles:

[nastiatikk]

<label>
    <input type="radio" class="radio" name="radio_button" value="radio_1"> 
    Radio 1
</label>

If existing styles ^ are gonna be our default styles (even if they're not used in the projects) then I think we can close this PR, and I will create select-box component where we could apply this markup and styles.

[jeffkamo]

I prefer option 2. That's also how we've it works in our field component.

[ry5n]

Same: no. 2.

[nastiatikk]

Well, in this case I need to change styles to match new markup and somehow avoid using float: left

[jeffkamo]

Hmm, I'm not sure you should worry about the radio/checkbox + label layout in vellum... I still think that can be the responsibilitity of c-field. Thoughts @ry5n?

[nastiatikk]

@jeffkamo If №2 is chosen to be Vellum markup, we will have this result without worrying about radio/checkbox + label:

[jeffkamo]

why not leave them as inline-block?

[nastiatikk]

Because with inline-block long labels will drop down

[ry5n]

My gut says this is a layout concern that probably doesn’t belong in Vellum. Checkboxes and radios are inline-block by default; we should probably leave them that way. I agree with @jeffkamo that a component should be responsible for more complex formatting of these elements together.

[nastiatikk]

It is layout concern, but this concern will not let us use default markup never. So what is the point of having this markup then or what is the point of having it broken?

[jeffkamo]

I don't really forsee us ever using these elements (checkbox/radio input + label) in this fashion in any projects going forward. So I don't really feel like it's an issue.

If it helps, you can think of Vellum styles as just the visual look of these things, rather than defining how two or more elements interact.

[kpeatt]

Definitely let's make a component that makes this better but it shouldn't be on Vellum to do it.

[nastiatikk]

Yes, it makes sense.

I've changed it and it can be reviewed again!

Changes:

• Following checkbox/radio label is display: inline-block now
• Test file uses №2 structure (see it above)
• Test file indents changed from 2 spaces to 4 spaces

[ry5n]

@nastiatikk Cool. Fixing the HTML in a separate PR is probably the right call. So then I think the only remaining issues:

• indentation.
• the checkbox + label style flagged as not belonging here

I haven’t had a chance to look at the rendered output since the last change but I’ll do that as soon as I can.

[jeffkamo]

Looking good. It appears you some merge conflicts to deal with, but probably when you have that cleaned up this will be good to merge. :+1: