-
Notifications
You must be signed in to change notification settings - Fork 8
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
New form builder #204
New form builder #204
Conversation
8e49558
to
a8652d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a review with Wave on the Review app. I ran into a little bit of trouble because the inputs all have the same name (with Wave really didn't like), so apologies if I interpreted it wrong.
-
Generally it looks like it performs a lot like v1. Which is great!
-
The label's "optional" text is outside the label. I'm not sure if that's intended, but the HTML was unexpected:
<label for="form_example_method_name">Example input with options</label> <div class="form-question--optional"></div> <span class="sr-only">(Optional)</span>
-
I'm most curious about the behavior of the "aria-describedby" and the interaction between errors and help_text. I dunno if I'm too in the weeds but I suspect there is some conflict between them.
-
As a breaking v2, I would like to give some input on the method signature of
cfa_text_input
because I think some significant changes would be really beneficial. Specifically:- Comparing the arguments to the comparable built-in Rails form_helper and trying to create as much symmetry as possible.
- having consistent
options
. For example, havinginput_options
,label_options
, (label_wrapper_options
?)group_options
- (technical/implementation) I am curious how constructing HTML with
capture
/concat
and Rails html helper methods (e.g.content_tag
), instead of using HTML heredocs, might potentially influence the method signatures. There are some tradeoffs, but I look atwrapper_classes
as being so unlike a Rails helper method that it makes me curious.
a8652d4
to
bab1162
Compare
b542997
to
d79f57c
Compare
459053c
to
445ba91
Compare
8a8e384
to
113428f
Compare
This is very high level observational feedback, non-blocking: in thinking about the goals that we set out "adhering as closely to the method options and signatures in the Rails FormBuilder as possible", I noticed that I think the form elements have a very similar interface to As it is, I am very happy with the new form builder 🎉 |
Ben Golder did a styling spike to see if we could style the form builder without adding style hook classes that exist in our CSS (see form-elements & form-molecules SCSS files). After trying this out, we felt it was better to add the style hook classes since it would get us closer to current styles faster (and otherwise we'd just be copying a lot of existing styles), and would also allow us still to override (eg by swapping out the SCSS files or by selectively targeting CSS). Here are the hook classes that I think we need to add, based on referencing from the styleguide documentation, form elements and form-molecules SCSS, and GetCalFresh: form elements:
form molecules
|
a517811
to
08d7221
Compare
Includes Form Builder and specs from https://github.com/codeforamerica/illinois_petition_service/blob/master/app/helpers/cmr_v2_form_builder.rb. Still to go: - Decide if we want to require label and legend text in method signatures - Add missing methods for Honeycrisp v1 FormBuilder (possibly: checkbox_set, checkbox_set_with_none, range_field, textarea, single_tap_button) - Bring over dense form styles - Bring over supporting files for new date input (validations, multipart form file) Co-authored-by: Christa Hartsock <[email protected]>
Adds text input Still needs: - Remaining implemented fields - Styles for form builder methods Co-authored-by: Ben Sheldon <[email protected]>
This allows us to use any method name prefixed with example_method as a method for a form, which will generate unique IDs. Unique IDs are necessary for doing realistic accessibility testing with Wave. [#175500754] Co-authored-by: Ben Sheldon <[email protected]>
- Move 'optional' tag into the label, so that it is picked up by screenreaders as being associated with the input - When an input is errored, add a text prefix to the label announcing that it's errored, so that the screenreader notifies the user without them having to listen to all assocaited descriptions. [#175500754] Co-authored-by: Ben Sheldon <[email protected]>
Also makes implementation more consistent with other form methods, including adding an external div with identifying class and then accepting wrapper classes. In future, may want to accept a block instead of label text, so that we can pass in things like icons. [#175500754]
This is a blueprint for changing the method signatures of all v2 form builder methods. Co-authored-by: Christa Hartsock <[email protected]>
And validate it. Also updates method names in the text-input partial so that IDs are unique on the documentation page. Co-authored-by: Christa Hartsock <[email protected]>
Co-authored-by: Christa Hartsock <[email protected]>
Can be used for radios, checkboxes, etc. Also removes CSS helping classes like form-question in favor of reducing HTML and writing our own CSS classes.
Doesn't show its own errors, since it's unclear what that would look like. Expectation is that it would only be used within a cfa_fieldset, which would render errors.
Does include error state, since checkboxes can exist without fieldset (eg for consent / confirmations) Required/disabled are handled by input options -- may need some styling hook for future, since it's hard to target a descendant CSS state (checked input) for a wrapping class.
We get this for free because we collect all input options as keyword args, and each of the inputs have a supported HTML attribute of 'required'. This means we also don't need to set "aria-required" since we can just use the HTML attribute. Also: - Adds in validation errors to examples
Naming convention more closely matches Rails FormBuilder
To match Rails FormBuilder naming convention.
Needs much more work to bundle with Honeycrisp gem. Intend to add after launch.
Should help us use existing styles, and build ones more easily in future. The included styles are based off of our form-elements and, selectively, some base form molecules (just .form-group, so that .form-group--error can style appropriately).
Javascript was only looking at the parent, but when there is an error on a checkbox Rails inserts a div between the errored input and where we are assigning the checkbox class. This brings the click handler in line with what we are doing on initial state setting.
08d7221
to
34e78d5
Compare
Should more easily support Honeycrisp compact checkbox styles
Should more easily support Honeycrisp compact checkbox styles
Molly, Symonne, and I created a new FormBuilder on CMR because we found the previous Honeycrisp version difficult to work with, especially when styling differently than GCF (CMR was using in a new 'compact form' context).
In creating this, CMR looked into existing Form Builders (Rails, Formtastic, GCF, Honeycrisp V1) to chart our course. The rough notes from that research are here. We also looked to past conversations about the future fo the Honeycrisp FormBuilder, which can be found here. here, and here.
For this reason, we decided to undertake building a new FormBuilder for Honeycrisp. Here are some goals we articulated for it:
Additionally, after talking with Ben S, I added the following goal:
Current roadmap / TODOs:
cfa_radiogroup
cfa_radio
cfa_button
cfa_collection_check_boxes
cfa_collection_radio_buttons
Before merging to trunk:
cfa_text_input
->cfa_text_field
Possible work after merging to trunk:
cfa_collection_select
cfa_textarea
cfa_grouped_collection_select
(all based on RailsFormBuilder
)Outstanding questions:
cfa_check_box
class? What about error classes (vs only showing them + message on fieldset)? [update: decided to not assign error classes on individual checkboxes or radios, but do have wrapping class]cfa_check_box
, which would make them a bit wonky if used inside a fieldset. Same forcfa_radio
. Should we only show errors when used with a fieldset? I believe this is Rails Formbuilder behavior, but we should check. What about error classes? What is the best visual + screenreader experience? [update: ditto above. label right now for fieldset is required, which makes validation on an individual checkbox a bit wonky but you can hide it (visually and for screen readers) with CSS, so seemed ok]form-group--error
on wrapping classes when the rendered HTML does?