From 151a53c894e12c7f360c9c44e4353f481bea1fb9 Mon Sep 17 00:00:00 2001 From: Joel Hawksley Date: Tue, 3 Mar 2020 12:20:01 -0700 Subject: [PATCH] Respond to feedback from code review --- README.md | 28 +------------------ lib/action_view/component.rb | 2 +- lib/view_component/base.rb | 6 ++-- .../path_container_component.html.erb | 2 +- .../components/validations_component.html.erb | 1 + test/app/components/validations_component.rb | 7 +++++ test/view_component/view_component_test.rb | 8 ++++++ 7 files changed, 22 insertions(+), 32 deletions(-) create mode 100644 test/app/components/validations_component.html.erb create mode 100644 test/app/components/validations_component.rb diff --git a/README.md b/README.md index 5c229876c..112ec3dfc 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# ActionView::Component +# ViewComponent A view component framework for Rails. **Current Status**: Used in production at GitHub. Because of this, all changes will be thoroughly vetted, which could slow down the process of contributing. We will do our best to actively communicate status of pull requests with any contributors. If you have any substantial changes that you would like to make, it would be great to first [open an issue](http://github.com/github/actionview-component/issues/new) to discuss them with us. @@ -107,8 +107,6 @@ Component class names end in -`Component`. Component module names are plural, as they are for controllers. (`Users::AvatarComponent`) -Components support ActiveModel validations. Components are validated after initialization, but before rendering. - Content passed to a `ViewComponent` as a block is captured and assigned to the `content` accessor. #### Quick start @@ -144,8 +142,6 @@ A `ViewComponent` is a Ruby file and corresponding template file (in any format `app/components/test_component.rb`: ```ruby class TestComponent < ViewComponent::Base - validates :content, :title, presence: true - def initialize(title:) @title = title end @@ -175,20 +171,6 @@ Which returns: Hello, World! ``` -#### Error case - -If the component is rendered with a blank title: - -```erb -<%= render(TestComponent.new(title: "")) do %> - Hello, World! -<% end %> -``` - -An error will be raised: - -`ActiveModel::ValidationError: Validation failed: Title can't be blank` - #### Content Areas A component can declare additional content areas to be rendered in the component. For example: @@ -196,8 +178,6 @@ A component can declare additional content areas to be rendered in the component `app/components/modal_component.rb`: ```ruby class ModalComponent < ViewComponent::Base - validates :user, :header, :body, presence: true - with_content_areas :header, :body def initialize(user:) @@ -246,8 +226,6 @@ This allows a few different combinations of ways to render the component: `app/components/modal_component.rb`: ```ruby class ModalComponent < ViewComponent::Base - validates :header, :body, presence: true - with_content_areas :header, :body def initialize(header:) @@ -272,8 +250,6 @@ end `app/components/modal_component.rb`: ```ruby class ModalComponent < ViewComponent::Base - validates :header, :body, presence: true - with_content_areas :header, :body def initialize(header: nil) @@ -308,8 +284,6 @@ end `app/components/modal_component.rb`: ```ruby class ModalComponent < ViewComponent::Base - validates :body, presence: true - with_content_areas :header, :body def initialize(header: nil) diff --git a/lib/action_view/component.rb b/lib/action_view/component.rb index 50c701a80..6613d33b6 100644 --- a/lib/action_view/component.rb +++ b/lib/action_view/component.rb @@ -1,4 +1,4 @@ - # frozen_string_literal: true +# frozen_string_literal: true require "active_model" require "view_component" diff --git a/lib/view_component/base.rb b/lib/view_component/base.rb index f9758595a..298267176 100644 --- a/lib/view_component/base.rb +++ b/lib/view_component/base.rb @@ -160,17 +160,17 @@ def compiled? end def compile! - compile(validate: true) + compile(raise_template_errors: true) end # Compile templates to instance methods, assuming they haven't been compiled already. # We could in theory do this on app boot, at least in production environments. # Right now this just compiles the first time the component is rendered. - def compile(validate: false) + def compile(raise_template_errors: false) return if compiled? if template_errors.present? - raise ViewComponent::TemplateError.new(template_errors) if validate + raise ViewComponent::TemplateError.new(template_errors) if raise_template_errors return false end diff --git a/test/app/components/path_container_component.html.erb b/test/app/components/path_container_component.html.erb index e99d8b163..e80bf3b4b 100644 --- a/test/app/components/path_container_component.html.erb +++ b/test/app/components/path_container_component.html.erb @@ -1 +1 @@ -<%= render PathComponent.new %> \ No newline at end of file +<%= render PathComponent.new %> diff --git a/test/app/components/validations_component.html.erb b/test/app/components/validations_component.html.erb new file mode 100644 index 000000000..352ce9db3 --- /dev/null +++ b/test/app/components/validations_component.html.erb @@ -0,0 +1 @@ +<%= content %> diff --git a/test/app/components/validations_component.rb b/test/app/components/validations_component.rb new file mode 100644 index 000000000..75f3df5b2 --- /dev/null +++ b/test/app/components/validations_component.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class ValidationsComponent < ActionView::Component::Base + validates :content, presence: true + + def initialize(*); end +end diff --git a/test/view_component/view_component_test.rb b/test/view_component/view_component_test.rb index 795755948..870c98e70 100644 --- a/test/view_component/view_component_test.rb +++ b/test/view_component/view_component_test.rb @@ -327,6 +327,14 @@ def test_no_validations_component assert_selector("div") end + def test_validations_component + exception = assert_raises ActiveModel::ValidationError do + render_inline(ValidationsComponent.new) + end + + assert_equal exception.message, "Validation failed: Content can't be blank" + end + private def modify_file(file, content)