Skip to content
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

Implement mount_with_defaults #1477

Closed
wants to merge 4 commits into from
Closed

Implement mount_with_defaults #1477

wants to merge 4 commits into from

Conversation

stephendolan
Copy link
Member

@stephendolan stephendolan commented Apr 20, 2021

Purpose

Fixes #1152 by adding a new mount_with_defaults method that can inject some helpful defaults into components.

Description

I'd like to get some feedback from others before I go too far down any given path. That said, I've got the thing that was blocking me working. Say we have a component with a form, like this:

class Filters::Form < BaseComponent
  needs context : HTTP::Server::Context
  needs current_user : User
  needs operation : SaveFilter
  needs route : Lucky::RouteHelper
  needs button_text : String

  def render
    form_for route, class: "divide-y divide-gray-200" do
      div class: "px-4 py-5 sm:p-6 space-y-4" do
        mount Filters::FormFields, operation
      end

      div class: "px-4 py-4 sm:px-6" do
        mount UI::Button, &.submit button_text
      end
    end
  end
end

This is a really nice abstraction, and greatly cleans up the duplication between edit_page.cr andnew_page.cr for the Filter model, in this case.

I'm not super thrilled about having to manually add needs context and needs current_user to the component, but also not sure if we can inject that after the component is initialized so that it's available but invisible.

The use of this component looks something like this:

class Filters::NewPage < MainLayout
  needs operation : SaveFilter
  quick_def page_title, "New Filter"

  def content
    div class: "bg-white overflow-hidden shadow rounded-lg divide-y divide-gray-200" do
      div class: "px-4 py-5 sm:px-6" do
        h3 class: "text-lg leading-6 font-medium text-gray-900" do
          text "New filter"
        end
      end

      mount_with_defaults Filters::Form, operation: operation, route: Filters::Create.route, button_text: "Create filter"
    end
  end
end

To-do

  • - Add specs for the new mount_with_defaults

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

@@ -22,7 +22,7 @@ module Lucky::Memoizable
macro memoize(method_def)
{% raise "You must define a return type for memoized methods" if method_def.return_type.is_a?(Nop) %}
{%
raise "All arguments must have an explicit type for memoized methods" if method_def.args.any? &.is_a?(Nop) # ameba:disable Style/IsAFilter
raise "All arguments must have an explicit type for memoized methods" if method_def.args.any? &.is_a?(Nop)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can break this out into a separate PR after we work through the rest of the stuff, but I wanted to get the CheckFormat specs passing while we're tinkering.

@stephendolan
Copy link
Member Author

I think I misunderstood what Paul/Jeremy were thinking about in the issue linked here, but I'm still happy to take this in the right direction with some guidance!

@stephendolan
Copy link
Member Author

This was instructive for me to poke around with, but I don't think it's a viable solution. Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement mount_defaults
1 participant