rails
rails copied to clipboard
Add custom_error to ActiveModel::Error and ActiveModel::Errors
- Summary
-
Context
- Listing errors at the top of a long form
-
Error per input field
- Current behaviour
-
Implemented behaviour: custom_message
- Usage
- Notes on implementation
- Future work
Summary
Note: This PR is a new take on https://github.com/rails/rails/pull/43283 by @ollieh-m based on the comments provided by me and @rafaelfranca. Note that the description of this PR is similar but not the same as the comment I made on that PR so please read again if you already read the other one :)
Context
This PR adds the custom_message
functionality to ActiveModel::Error
and helper methods in ActiveModel::Errors
in an effort to make feedback in form fields nicer.
Listing errors at the top of a long form
When creating a form, specially a long one, it's usually a good idea for all errors to be at the top of the form. This makes it specially important for users of your app that use assistive technologies (more on that here)
Each item in this list describes a) the attribute that has an error and b) the error on that attribute. Each list item can potentially have a link with an anchor to the element that needs to be corrected
This message is currently constructed using the full_message
method which concatenates the name of the attribute with the error message.
Error per input field
Current behaviour
As UX on the web has evolved, it's become commonplace to have the error message alongside the input field. This makes it a little bit redundant to have the name of the attribute be repeated. The following example showcases what an input field would look like using the full_messages_for(:attribute)
method.
Implemented behaviour: custom_message
I've added a custom_message
method to create an alternative message in forms like these that don't need the attribute's name or might not follow the format attribute + message. In working in this PR I discovered a feature in full_messages
that allows its format to be modified from #{name} #{message}
to something else and also a PR https://github.com/rails/rails/pull/42708 that allows this configuration to be made per validation.
However, neither of these accomplish the purpose of this feature to have 2 distinct ways to describe error messages: one conventionally formatted with full_message
and one friendlier with custom_message
.
Usage
<%= form_with(model: @person) do |form|%>
<!-- for accessibility -->
<ul>
<% form.object.errors.full_messages.each do |message| %>
<li><%= message %></li>
</ul>
<%= form.email_field :email %>
<p class="error"><%= form.object.errors.custom_message_for(:email) %></p>
<% end %>
Notes on the implementation
- I based the implementation and tests for the
ActiveModel::Error
class on themessage
andgenerate_message
already present. - I decided to go with a new structure for the YAML translation file where the type of error (e.g.
blank
) is not a single key with a string value but a key with child keys:blank: { message: "can't be blank", custom_message: "Whoops! it's blank!" }
. This required for the lookup chain formessage
to support the previous, simpler nesting:blank: "can't be blank"
. I've commented this in the lookup chain docs inErrors
in case we wish to deprecate this in the future. - I enhanced the
as_json
andto_hash
methods to support a more explicit type of argument:message_method
. This allows callers to ask for the type of message they want, eithermessage
,custom_message
andfull_message
. I've added deprecation warnings for users to upgrade to using explicit keyword arguments for as_json
Future work
- The
Error
class and specially its tests are quite long. I'm personally not used to this and thought it could be good to split the rendering ofmessage
andcustom_message
into different classes likeMessageRenderer
andCustomMessageRenderer
or something similar. - I wonder it's necessary to deprecate the previous lookup chain values
- @rafaelfranca made an interesting comment on
full_message
not being the right name for what it does and I agree.formatted_message
as he suggested describes its behaviour and intention much better. I wonder if we should take this opportunity to rename it and create a deprecation warning forfull_message
Other Information
TODO: If you are updating any of the CHANGELOG files or are asked to update the CHANGELOG files by reviewers, please add the CHANGELOG entry at the top of the file.
Finally, if your pull request affects documentation or any non-code changes, guidelines for those changes are available here
Thanks for contributing to Rails! -->
Hi @pinzonjulian, If you used code from @ollieh-m, could you add them as a coauthor in a commit?
Co-Authored-By: ....
Could you also squash the commits?
I actually didn't use any code from his implementation but happy to add him for coming up with the initial implementation.
Squashed and implemented your suggestions @p8.
Hey! Happy new year :)
Friendly bump.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Bump! @p8 @rafaelfranca could you take a look please?
Thanks for your comments @ollieh-m. Really appreciate them.
Before working more on this though, I'd like for @p8 or @rafaelfranca the to give their opinions regarding the fallback chains for these. I wouldn't want to change it without some more feedback.