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

Add options contexts to set conventions for more involved definitions #3

Merged
merged 20 commits into from Feb 20, 2023

Conversation

kaspth
Copy link
Collaborator

@kaspth kaspth commented Jan 6, 2023

No description provided.

@seanpdoyle
Copy link
Collaborator

seanpdoyle commented Feb 6, 2023

This is a really interesting idea! Is a Context something that's internal to Showcase, or would applications want to define their own too?

For example, I think an aria- context might be useful to support too, in addition to Stimulus (see hotwired/stimulus#627 for an example where they might overlap in the future).

@kaspth
Copy link
Collaborator Author

kaspth commented Feb 6, 2023

This is a really interesting idea! Is a Context something that's internal to Showcase, or would applications want to define their own too?

Yeah, the idea is they're able to do something like this:

Showcase.options.context :stimulus do
  def value(name, ...)
    option("data-#{@controller}-#{name}-value", ...)
  end
end

and then showcase.options.stimulus controller: :welcome works in their Showcases. Of course, Stimulus support is shipped out of the box.

For example, I think an aria- context might be useful to support too, in addition to Stimulus,

Oh yeah, it'd be interesting to ship something like this out of the box too.

* main: (21 commits)
  Prep v0.2.0 release
  Ensure test support for deeply nested previews
  Rename `Page` to `Preview` (#17)
  Route Engine root path to `showcase/engines#index` (#18)
  Add a description to our Plain Ruby sample
  Load with Zeitwerk (#14)
  Support partials with host application routes
  Bugfix: support deeply-nested partials in IntegrationTest
  Convert Plain Ruby sample to new partial location and have it auto-smoketested 😁
  Add automatic Smokescreen test generation (#13)
  Indent Path Tree children
  Test Coverage for template- and partial-overrides
  Move `pages#index` template to `engine#index`
  Revert `<input type="checkbox" readonly>` change
  Build Tailwind assets
  Fail CI when Tailwind generates a diff
  Add HTML Test Coverage
  Execute Test Suite in GitHub Action (#7)
  Prefer partials to templates (#2)
  Prefix Tailwind classes with sc- to avoid clashing with app Tailwind (#4)
  ...
@kaspth
Copy link
Collaborator Author

kaspth commented Feb 7, 2023

We'll end up rendering the Stimulus specific options like this now:

Screen Shot 2023-02-07 at 01 02 51

@kaspth
Copy link
Collaborator Author

kaspth commented Feb 7, 2023

Looks like the build is failing on with_options not being chainable in 6.1. I also had to use o.required.klass to get things working which isn't really that nice. Not sure whether we should chug the with_options support. I think I'll sleep on it, unless you've got thoughts @seanpdoyle.

@seanpdoyle
Copy link
Collaborator

@kaspth what about a guarded backport that stashes the block argument?

require "active_support/core_ext/object/with_options"

module WithOptionsBackport
  extend ActiveSupport::Concern

  included do
    alias_method :__original_with_options, :with_options

    def with_options(options, &block)
      if block.nil?
        options_merger = nil
        __original_with_options(options) { |object| options_merger = object }
        options_merger
      else
        __original_with_options(options, &block)
      end
    end
  end
end

if Rails.version < "7.0"
  Object.include WithOptionsBackport
end

It's brittle, but the behavior itself isn't too complicated to cover.

@kaspth
Copy link
Collaborator Author

kaspth commented Feb 7, 2023

I noticed that with_options without a block just returns an ActiveSupport::OptionMerger instance, so we can just use that directly instead of mucking with with_options.

test/controllers/showcase/previews_controller_test.rb Outdated Show resolved Hide resolved
lib/showcase.rb Show resolved Hide resolved
@contexts = Hash.new { |h,k| h[k] = Class.new Context }

def self.define(key, &block)
contexts[key].class_eval(&block) # Lets users reopen an already defined context class.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, if people override an already defined method they may break Showcases from engines, which may rely on the default context method definition.

Comment on lines +17 to +22
<% showcase.options.context :stimulus, controller: :welcome do |o| %>
<% o.optional.targets :greeter, "If the id of the target element must be printed" %>
<% o.required.values :yell, "Whether the hello is to be YELLED", default: false %>
<% o.required.classes :success, "The success class to append after greeting" %>
<% o.required.outlet :list, "An outlet to append each yelled greeter to" %>
<% o.optional.action :greet, "An action to repeat the greeting, if need be" %>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This feels pretty clean and intention revealing!

* main:
  Add basic reveal of render time and allocation count (#22)
  Test showcasing a helper (#26)
  Prep v0.2.1
  Yank Zeitwerk for now (#24)
  Run CI with Ruby 2.7 since we still need to support it (#21)
  Add automatic integration testing via autorun (#16)
@kaspth
Copy link
Collaborator Author

kaspth commented Feb 13, 2023

I've gone back to push more on the Bullet Train integration side of things bullet-train-co/bullet_train-core#46, so I'll be needing this soon. I'll try to document this soon and then go ahead with what we've got.

@kaspth kaspth merged commit 7ac7144 into main Feb 20, 2023
@kaspth kaspth deleted the options-contexts branch February 20, 2023 01:53
kaspth added a commit that referenced this pull request Feb 20, 2023
* main:
  Try latest rubygems with bundler 2.4 to see if Rails main + Ruby 2.7 bundles faster (#34)
  Add options contexts to set conventions for more involved definitions (#3)
  Fix assets rendering paths (#33)
  Prep v0.2.2
  Finalize default badge support (#32)
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.

None yet

2 participants