From cde76e66f68487b9dfeb3cfaf168d1617749485a Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 22 Feb 2021 01:36:54 -0600 Subject: [PATCH] forms: fix form validation error messages. * Fix it so that all edit forms show an error banner if the form has validation errors. Previously forms had to manually call `error_messages_for`, which not all forms did. * Fix it so that the full validation error message is shown next to each input attribute that had errors. Also update the styling of these error messages to look better. --- app/helpers/application_helper.rb | 19 ++++++++----------- app/javascript/src/styles/base/040_colors.css | 6 ++---- app/javascript/src/styles/common/inline.scss | 6 ------ app/javascript/src/styles/common/notices.scss | 10 ---------- .../src/styles/common/simple_form.scss | 13 +++++++++++-- app/views/artists/new.html.erb | 2 -- app/views/bans/_form.html.erb | 2 -- app/views/bans/edit.html.erb | 2 -- app/views/bulk_update_requests/_form.html.erb | 2 -- app/views/comments/_form.html.erb | 2 -- app/views/favorite_groups/edit.html.erb | 2 -- app/views/favorite_groups/new.html.erb | 2 -- .../forum_posts/partials/edit/_form.html.erb | 2 -- .../forum_posts/partials/new/_form.html.erb | 2 -- app/views/forum_topics/_form.html.erb | 2 -- app/views/ip_bans/new.html.erb | 2 -- app/views/pools/edit.html.erb | 2 -- app/views/saved_searches/edit.html.erb | 2 -- app/views/user_feedbacks/edit.html.erb | 2 -- app/views/user_feedbacks/new.html.erb | 2 -- .../user_name_change_requests/new.html.erb | 2 -- app/views/wiki_pages/_form.html.erb | 2 -- app/views/wiki_pages/new.html.erb | 2 -- config/initializers/simple_form.rb | 6 +++--- 24 files changed, 24 insertions(+), 72 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 1057a076f..ddd15a408 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -92,16 +92,6 @@ module ApplicationHelper DText.strip_dtext(text) end - def error_messages_for(instance_name) - instance = instance_variable_get("@#{instance_name}") - - if instance&.errors&.any? - %{
Error: #{instance.__send__(:errors).full_messages.join(", ")}
}.html_safe - else - "" - end - end - def time_tag(content, time, **options) datetime = time.strftime("%Y-%m-%dT%H:%M%:z") @@ -256,7 +246,14 @@ module ApplicationHelper def edit_form_for(model, **options, &block) options[:html] = { autocomplete: "off", **options[:html].to_h } options[:authenticity_token] = true if options[:remote] == true - simple_form_for(model, **options, &block) + + simple_form_for(model, **options) do |form| + if model.try(:errors).try(:any?) + concat tag.div(model.errors.full_messages.join("; "), class: "notice notice-error notice-small") + end + + block.call(form) + end end def table_for(...) diff --git a/app/javascript/src/styles/base/040_colors.css b/app/javascript/src/styles/base/040_colors.css index c480df553..83b141c35 100644 --- a/app/javascript/src/styles/base/040_colors.css +++ b/app/javascript/src/styles/base/040_colors.css @@ -114,6 +114,7 @@ html { --form-input-border-color: var(--grey-2); --form-input-placeholder-text-color: var(--grey-4); --form-input-validation-error-border-color: var(--red-4); + --form-input-validation-error-text-color: var(--red-5); --form-button-text-color: var(--text-color); --form-button-background: var(--grey-1); @@ -246,8 +247,6 @@ html { --keyboard-shortcut-color: var(--inverse-text-color); --keyboard-shortcut-background-color: var(--grey-6); - --error-message-color: var(--red-5); - --login-link-color: var(--red-5); --footer-border-color: var(--grey-1); --details-border-color: var(--grey-2); @@ -326,6 +325,7 @@ body[data-current-user-theme="dark"] { --form-input-border-color: var(--grey-5); --form-input-placeholder-text-color: var(--grey-3); --form-input-validation-error-border-color: var(--red-4); + --form-input-validation-error-text-color: var(--red-5); --form-button-text-color: var(--grey-9); --form-button-background: var(--grey-2); @@ -444,8 +444,6 @@ body[data-current-user-theme="dark"] { --keyboard-shortcut-color: var(--grey-0); --keyboard-shortcut-background-color: var(--grey-7); - --error-message-color: var(--red-4); - --login-link-color: var(--red-4); --footer-border-color: var(--grey-7); --details-border-color: var(--grey-7); diff --git a/app/javascript/src/styles/common/inline.scss b/app/javascript/src/styles/common/inline.scss index a68b08f34..d2f831de1 100644 --- a/app/javascript/src/styles/common/inline.scss +++ b/app/javascript/src/styles/common/inline.scss @@ -1,9 +1,3 @@ -span.error { - display: block; - font-weight: bold; - color: var(--error-color); -} - span.link { color: var(--link-color); cursor: pointer; diff --git a/app/javascript/src/styles/common/notices.scss b/app/javascript/src/styles/common/notices.scss index 1e0b36a76..5684b97f0 100644 --- a/app/javascript/src/styles/common/notices.scss +++ b/app/javascript/src/styles/common/notices.scss @@ -1,13 +1,3 @@ -div.error-messages { - margin: 1em 0; - padding: 1em; - - h1 { - font-size: 1em; - color: var(--error-message-color); - } -} - div#notice { padding: 0.5em; position: fixed; diff --git a/app/javascript/src/styles/common/simple_form.scss b/app/javascript/src/styles/common/simple_form.scss index 5ee01139b..2d290762f 100644 --- a/app/javascript/src/styles/common/simple_form.scss +++ b/app/javascript/src/styles/common/simple_form.scss @@ -44,8 +44,17 @@ form.simple_form { } } - &.field_with_errors input { - border: 1px solid var(--form-input-validation-error-border-color); + &.field_with_errors { + input, select, textarea { + border: 1px solid var(--form-input-validation-error-border-color); + } + + span.error { + display: block; + color: var(--form-input-validation-error-text-color); + font-style: italic; + font-size: 0.8em; + } } &.text, &.dtext { diff --git a/app/views/artists/new.html.erb b/app/views/artists/new.html.erb index bc37bbebc..f0a620466 100644 --- a/app/views/artists/new.html.erb +++ b/app/views/artists/new.html.erb @@ -2,8 +2,6 @@

New Artist

- <%= error_messages_for :artist %> - <%= render "form" %>
diff --git a/app/views/bans/_form.html.erb b/app/views/bans/_form.html.erb index 5667aa73f..ec18d0df6 100644 --- a/app/views/bans/_form.html.erb +++ b/app/views/bans/_form.html.erb @@ -1,6 +1,4 @@ <%= edit_form_for(ban) do |f| %> - <%= error_messages_for("ban") %> - <%= f.input :user_name, :as => :string, :input_html => { data: { autocomplete: "user" } } %> <%= f.input :duration, :hint => "in days" %> <%= f.input :reason %> diff --git a/app/views/bans/edit.html.erb b/app/views/bans/edit.html.erb index 11cfdd309..bb4ddefa2 100644 --- a/app/views/bans/edit.html.erb +++ b/app/views/bans/edit.html.erb @@ -3,8 +3,6 @@

Edit Ban

<%= edit_form_for(@ban) do |f| %> - <%= error_messages_for("ban") %> - <%= f.input :duration, :hint => "in days" %> <%= f.input :reason %> <%= f.button :submit, :value => "Ban" %> diff --git a/app/views/bulk_update_requests/_form.html.erb b/app/views/bulk_update_requests/_form.html.erb index 8473fb1d8..8e654913f 100644 --- a/app/views/bulk_update_requests/_form.html.erb +++ b/app/views/bulk_update_requests/_form.html.erb @@ -1,6 +1,4 @@ <%= edit_form_for(@bulk_update_request) do |f| %> - <%= error_messages_for("bulk_update_request") %> -

Request aliases or implications using the format shown below. An alias makes the first tag a synonym for the second tag. An implication makes the first tag automatically add the second tag. diff --git a/app/views/comments/_form.html.erb b/app/views/comments/_form.html.erb index 33ff69df8..01d91892c 100644 --- a/app/views/comments/_form.html.erb +++ b/app/views/comments/_form.html.erb @@ -1,5 +1,3 @@ -<%= error_messages_for :comment %> - <%= edit_form_for(comment, namespace: "post_#{comment&.post_id}_comment_#{comment.id || "new"}", html: { style: ("display: none;" if local_assigns[:hidden]), class: "edit_comment" }) do |f| %> <% if comment.new_record? %> <%= f.hidden_field :post_id %> diff --git a/app/views/favorite_groups/edit.html.erb b/app/views/favorite_groups/edit.html.erb index ac6e706bc..29d7369eb 100644 --- a/app/views/favorite_groups/edit.html.erb +++ b/app/views/favorite_groups/edit.html.erb @@ -2,8 +2,6 @@

Edit Favorite Group: <%= @favorite_group.pretty_name %>

- <%= error_messages_for "favorite_group" %> - <%= edit_form_for(@favorite_group) do |f| %> <%= f.input :name, :as => :string, :input_html => { :value => @favorite_group.pretty_name } %> <%= f.input :post_ids_string, label: "Posts", as: :text %> diff --git a/app/views/favorite_groups/new.html.erb b/app/views/favorite_groups/new.html.erb index 8f6724b53..b3245edaa 100644 --- a/app/views/favorite_groups/new.html.erb +++ b/app/views/favorite_groups/new.html.erb @@ -2,8 +2,6 @@

New Favorite Group

- <%= error_messages_for "favorite_group" %> - <%= edit_form_for(@favorite_group) do |f| %> <%= f.input :name, as: :string, required: true %> <%= f.input :post_ids_string, label: "Posts", as: :text %> diff --git a/app/views/forum_posts/partials/edit/_form.html.erb b/app/views/forum_posts/partials/edit/_form.html.erb index 9dfd75b26..a41aa569d 100644 --- a/app/views/forum_posts/partials/edit/_form.html.erb +++ b/app/views/forum_posts/partials/edit/_form.html.erb @@ -1,5 +1,3 @@ -<%= error_messages_for("forum_post") %> - <%= edit_form_for(forum_post, namespace: "forum_post_#{forum_post.id}") do |f| %> <%= f.input :body, as: :dtext %> diff --git a/app/views/forum_posts/partials/new/_form.html.erb b/app/views/forum_posts/partials/new/_form.html.erb index cbfa63350..a9ae00eda 100644 --- a/app/views/forum_posts/partials/new/_form.html.erb +++ b/app/views/forum_posts/partials/new/_form.html.erb @@ -1,5 +1,3 @@ -<%= error_messages_for("forum_post") %> - <%= edit_form_for(forum_post) do |f| %> <% if forum_post.topic_id.present? %> <%= f.input :topic_id, :as => :hidden %> diff --git a/app/views/forum_topics/_form.html.erb b/app/views/forum_topics/_form.html.erb index 468befdca..a8281c9f6 100644 --- a/app/views/forum_topics/_form.html.erb +++ b/app/views/forum_topics/_form.html.erb @@ -1,5 +1,3 @@ -<%= error_messages_for("forum_topic") %> -
<%= edit_form_for(forum_topic) do |f| %> <%= f.input :title %> diff --git a/app/views/ip_bans/new.html.erb b/app/views/ip_bans/new.html.erb index 7303a751a..448f5d9b2 100644 --- a/app/views/ip_bans/new.html.erb +++ b/app/views/ip_bans/new.html.erb @@ -12,8 +12,6 @@ they've verified their email address.

- <%= error_messages_for "ip_ban" %> - <%= edit_form_for(@ip_ban) do |f| %> <%= f.input :ip_addr, label: "IP Address", as: :string, hint: "Add /24 to ban a subnet. Example: 1.2.3.4/24" %> <%= f.input :reason, as: :string %> diff --git a/app/views/pools/edit.html.erb b/app/views/pools/edit.html.erb index 2c2652d6a..c4eedcc35 100644 --- a/app/views/pools/edit.html.erb +++ b/app/views/pools/edit.html.erb @@ -3,8 +3,6 @@ <%= edit_form_for(@pool) do |f| %>

Edit Pool: <%= @pool.pretty_name %>

- <%= error_messages_for "pool" %> - <%= f.input :name, :as => :string, :input_html => { :value => @pool.pretty_name } %> <%= f.input :description, as: :dtext %> <%= f.input :post_ids_string, as: :text, label: "Posts" %> diff --git a/app/views/saved_searches/edit.html.erb b/app/views/saved_searches/edit.html.erb index c612afa65..a02deaac3 100644 --- a/app/views/saved_searches/edit.html.erb +++ b/app/views/saved_searches/edit.html.erb @@ -2,8 +2,6 @@

Edit Saved Search

- <%= error_messages_for :saved_search %> - <%= edit_form_for(@saved_search) do |f| %> <%= f.input :query, :as => :string %> <%= f.input :label_string, label: "Labels", hint: "A list of tags to help categorize this search. Space delimited.", input_html: { "data-autocomplete": "saved-search-label" } %> diff --git a/app/views/user_feedbacks/edit.html.erb b/app/views/user_feedbacks/edit.html.erb index 9758a573d..4cfaee853 100644 --- a/app/views/user_feedbacks/edit.html.erb +++ b/app/views/user_feedbacks/edit.html.erb @@ -5,8 +5,6 @@
- <%= error_messages_for "user_feedback" %> - <%= edit_form_for(@user_feedback) do |f| %> <%= f.input :category, :collection => ["positive", "neutral", "negative"], :include_blank => false %> <%= f.input :body, as: :dtext %> diff --git a/app/views/user_feedbacks/new.html.erb b/app/views/user_feedbacks/new.html.erb index debad85a6..079e78516 100644 --- a/app/views/user_feedbacks/new.html.erb +++ b/app/views/user_feedbacks/new.html.erb @@ -11,8 +11,6 @@
- <%= error_messages_for "user_feedback" %> - <%= edit_form_for(@user_feedback) do |f| %> <%= f.input :user_name, :label => "User", :input_html => { value: @user_feedback.user.try(:name), data: { autocomplete: "user" } } %> <%= f.input :category, :collection => ["positive", "neutral", "negative"], :include_blank => false %> diff --git a/app/views/user_name_change_requests/new.html.erb b/app/views/user_name_change_requests/new.html.erb index 69f1db5a1..b1be8501d 100644 --- a/app/views/user_name_change_requests/new.html.erb +++ b/app/views/user_name_change_requests/new.html.erb @@ -6,8 +6,6 @@ be visible on your profile to other Danbooru members, but they won't be visible to search engines.

- <%= error_messages_for "change_request" %> - <%= edit_form_for(@change_request) do |f| %> <%= f.input :desired_name, label: "Name" %> <%= f.input :desired_name_confirmation, label: "Confirm name" %> diff --git a/app/views/wiki_pages/_form.html.erb b/app/views/wiki_pages/_form.html.erb index 5db3146bd..d987fd6eb 100644 --- a/app/views/wiki_pages/_form.html.erb +++ b/app/views/wiki_pages/_form.html.erb @@ -1,6 +1,4 @@
- <%= error_messages_for("wiki_page") %> - <%= edit_form_for(@wiki_page, url: wiki_page_path(@wiki_page.id)) do |f| %> <%= f.input :title, error: false, input_html: { data: { autocomplete: "tag" } }, hint: "Change to rename this wiki page. Update any wikis linking to this page first." %> diff --git a/app/views/wiki_pages/new.html.erb b/app/views/wiki_pages/new.html.erb index a212649b3..f5077534b 100644 --- a/app/views/wiki_pages/new.html.erb +++ b/app/views/wiki_pages/new.html.erb @@ -9,8 +9,6 @@
<% end %> - <%= error_messages_for("wiki_page") %> - <%= edit_form_for(@wiki_page) do |f| %> <%= f.input :title, error: false, input_html: { data: { autocomplete: "tag" } } %> <%= f.input :other_names_string, as: :string, label: "Other names (#{link_to_wiki "help", "help:translated_tags"})".html_safe, hint: "Names used for this tag on other sites such as Pixiv. Separate with spaces." %> diff --git a/config/initializers/simple_form.rb b/config/initializers/simple_form.rb index 2aaf05ca0..f3a65d9e3 100644 --- a/config/initializers/simple_form.rb +++ b/config/initializers/simple_form.rb @@ -57,13 +57,13 @@ SimpleForm.setup do |config| # b.use :input, class: 'input', error_class: 'is-invalid', valid_class: 'is-valid' b.use :label_input b.use :hint, wrap_with: { tag: :span, class: :hint } - b.use :error, wrap_with: { tag: :span, class: :error } + # b.use :error, wrap_with: { tag: :span, class: :error } ## full_messages_for # If you want to display the full error message for the attribute, you can # use the component :full_error, like: # - # b.use :full_error, wrap_with: { tag: :span, class: :error } + b.use :full_error, wrap_with: { tag: :span, class: :error } end # The default wrapper to be used by the FormBuilder. @@ -87,7 +87,7 @@ SimpleForm.setup do |config| # Method used to tidy up errors. Specify any Rails Array method. # :first lists the first message for each field. # Use :to_sentence to list all errors for each field. - # config.error_method = :first + config.error_method = :to_sentence # Default tag used for error notification helper. config.error_notification_tag = :div