aviflombaum / shadcn-rails

https://shadcn.rails-components.com
MIT License
470 stars 32 forks source link

Conflicting `list_item` Helper Methods in `filter_helper.rb` and `list_helper.rb` #64

Open eerkmen opened 1 month ago

eerkmen commented 1 month ago

Description

Currently, there are two list_item helper methods in the codebase, leading to confusion and potential misuse:

  1. filter_helper.rb:

    def list_item(value:, name:, selected:)
      "#{name}"
    end
  2. list_helper.rb:

    def list_item(value:, name:, selected: false, as: :div)
      content_tag as, value,
        class: "relative flex cursor-default select-none items-center rounded-sm px-2 py-1.5 text-sm
         outline-none aria-selected:bg-accent aria-selected:text-accent-foreground hover:bg-accent hover:text-accent-foreground
         data-[disabled]:pointer-events-none data-[disabled]:opacity-50",
        role: "option",
        data: {value:, selected:},
        aria: {selected:}
    end

The method in list_helper.rb is more complete, providing proper styling and functionality. However, the method in filter_helper.rb is simpler and lacks the necessary styling, leading to potential issues when developers use it inadvertently.

Background

As of the pull request fix: fixes syntax error in filter_helper #61 , the syntax error in filter_helper.rb was fixed, but the fundamental issue of having two list_item methods remains unresolved. It is not clear which helper method should be used or intended to be used, causing confusion.

Problem

Proposal

We need to decide on a single list_item method to be used across the application. Given that the list_helper.rb method is more complete and functional, it would be logical to adopt this as the standard.

Two possible solutions:

  1. Adopt the list_item Method from list_helper.rb:

    • Update all instances to use the list_item method from list_helper.rb.
    • Deprecate and remove the list_item method from filter_helper.rb.
  2. Use render_list Method Inside Filter Component View Partial:

    • Ensure the _filter.html.erb view uses render_list as a partial to indicate the dependency on the list component.
    • Evaluate if the list_item method in filter_helper.rb is necessary if the intended component to be used is list and its helper.

Request for Feedback

I am willing to make the necessary changes after addressing the intent and further plans for this issue. Please provide feedback on:

Once a decision is made, I can proceed with the implementation.