-
Notifications
You must be signed in to change notification settings - Fork 116
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
Use a custom form builder; introduce primer_form_for #1270
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: ac3a5b9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…components into dont_classify_twice
camertron
requested review from
a team,
keithamus and
jonrohan
and removed request for
keithamus
August 3, 2022 21:25
jonrohan
approved these changes
Aug 3, 2022
joelhawksley
approved these changes
Aug 4, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In attempting to upgrade to PVC v0.0.86 in dotcom, @jonrohan and I ran into a few issues. Version 0.0.86 includes all the re-homed PRF code, which is the main source of friction.
Previously, PRF handled running
Primer::Classify.call
on system arguments in theInput
class, which converts system arugments likeml: 3
into their corresponding CSS class counterparts. This approach works for all inputs except submit buttons, since submit buttons usePrimer::ButtonComponent
under the hood. The formSubmtButton
is anInput
, so classification happens at theInput
level and then again when all the system arguments are eventually passed toPrimer::ButtonComponent
. In the development and test environments, classify willraise
if it determines you includedml-3
in the list of classes instead of passing it asml: 3
in the system arguments. This led to errors appearing in dotcom test output since the first classify run done inInput
had already converted system arguments to class names before passing them toPrimer::ButtonComponent
.I experimented a bit with a flag in the
SubmitButton
input that disabled classification for the system arguments attached to<input>
elements, but realized the forms code runs the same classification process on system arguments attached to labels, etc. The flag seemed like an incomplete solution and a hacky one at that.Instead I decided to introduce a custom form builder that classifies system arguments right before passing them to rails to generate the
<input>
elements. I also introduced a helper method,primer_form_with
, that mimics rails'form_with
helper but automatically uses the custom builder. This is something I've wanted to do for a long time. It also lets us setskip_default_ids
automatically, which is important for a11y reasons.