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

Simplify generating form fields with labels #3745

Merged
merged 8 commits into from
Oct 6, 2019
Merged

Simplify generating form fields with labels #3745

merged 8 commits into from
Oct 6, 2019

Conversation

javierm
Copy link
Member

@javierm javierm commented Oct 5, 2019

Background

There's a pattern we used to use a lot:

f.label attribute, "Text"
f.text_field attribute, label: false

A year ago we decided to use this pattern instead:

f.text_field attribute, label: "Text"

Objectives

  • Remove legacy uses of our old pattern to generate a form field with a label
  • Add the label for attribute automatically when we specify the field's id attribute
  • Simplify generating fields with hints
  • Add the aria-describedby attribute automatically to fields with hints
  • Simplify generating a <span class="checbox"> to checkbox labels

Instead of generating the label and then a field without a label, we can
directly generate a field with a label.
We were monkey-patching FoundationRailsHelper::Formbuilder, which made
form customization difficult. We can inherit from it, which is the
standard way of extending what an existing class does, and make our form
the default one.
@javierm javierm self-assigned this Oct 5, 2019
@javierm javierm added this to Reviewing in Roadmap via automation Oct 5, 2019
@javierm javierm force-pushed the auto_labels branch 2 times, most recently from d9e09fd to 24c18f4 Compare October 6, 2019 10:41
We were already using this code in translatable forms. Using it on every
form means we can reduce the code we need to generate a field with a
hint.
We were manually adding the attribute in many places, but not
everywhere. I'm assuming adding it where we didn't have it is doing no
harm.
`select_tag` creates just a select with no label, so there's no need to
specify `label: false`.
Using the block syntax to generate the label with a <span> tag inside
isn't necessary after upgrading foundation_rails_helpers. Before the
upgrade, we couldn't do so because the <span> tag was escaped.
I'm not sure why it isn't already done by foundation's form builder. It
doesn't make any sense to change an ID of a form field without changing
the `for` attribute of its label.
@javierm javierm merged commit 24ccee0 into master Oct 6, 2019
Roadmap automation moved this from Reviewing to Release 1.1.0 Oct 6, 2019
@javierm javierm deleted the auto_labels branch October 6, 2019 18:34
smarques pushed a commit to venetochevogliamo/consul that referenced this pull request Apr 29, 2020
Simplify generating form fields with labels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Roadmap
  
Release 1.1.0
Development

Successfully merging this pull request may close these issues.

None yet

1 participant