coingaming / moon

Moon Design System for Elixir
https://surface.moon.io/
MIT License
195 stars 17 forks source link

Vanilla LiveView Form Inputs API Proposal #814

Closed gfrancischelli closed 7 months ago

gfrancischelli commented 8 months ago

This is a proposal for the Form Inputs API, which is currently being migrated from Surface to vanilla LiveView (#810)

TOC

  1. Need
  2. API Proposal
  3. Adoption Strategy
  4. Context
  5. Alternatives
  6. About this Document

Need

Moon form input components must fulfil the following requirements:

The non-goals are:

API Proposal

TLDR

Simple Use Case

The switch_field/1 should:

<.switch_field label="I agree terms of service" field={form[:agrees_to_terms_of_service]} />

Basic Customization

Simple approach: expose child assigns in switch_field/1

The switch_field/1 can also expose a size assign which is forwarded to the label. What else the size could belong to?

<.switch_field size="label_size" />

Advanced Customization

[!WARNING] APIs can only offer so much customization without becoming a bloated mess of complexity.

Simple approach: provide tools and building blocks to assemble entirely new components

Instead of chasing the holy grail of extensibility, MoonDS could provide the basic building blocks necessary for consumers to quickly assemble their own alternative version of every component.

Example:

def switch_field(assigns) do
  assigns = assigns_from_field(assigns)
  ~H"""
  <div phx-feedback-for={@name}> 
    <.label>
      <.switch_input name={@name} value={@value} /> <%= @label %>
    </.label>
    <.error :for={msg <- @errors}><%= msg %></.error>
  </div>
  """
end

def assigns_from_field(%{field: %Phoenix.HTML.FormField{} = field} = assigns) do
  assigns
  |> assign(field: nil, id: assigns.id || field.id)
  |> assign(:errors, Enum.map(field.errors, &translate_error/1))
  |> assign_new(:name, fn -> field.name end)
  |> assign_new(:value, fn -> field.value end)
end

When switch_field is composed of well-abstracted building blocks like .label, .error, .switch_input and assigns_from_field, consumers can effortlessly assemble their own alternative version my_custom_switch_field.

Allowing switch_field, checked_star_switch_field, sparks_switch_field to be written with the same building blocks removes a ton of complexity and mental gymnastics necessary for chasing endless customization.

Adoption Strategy

Successfull adoption depends on running this proposal through:

Giving consumers a chance and structure to provide feedback and express concerns is an act of respect. Once the benefits of the approach are clearer to consumers the drawbacks of adopting a new version will be more palatable.

Possible adoption path

[!WARNING] As an engineering organization we can't fall victim to the sunken cost falacy. Maintaning 1:1 parity with the old API is both:

  • undesirable because of the problems mentioned above
  • unfeasible because LiveView lacks a Context API

Context

Moon is currently being ported from Surface to vanilla LiveView. The migration strategy is to keep a 1:1 parity with the old API (correct if I'm wrong), leveraging the use of a translator script which converts Surface code to a similar vanilla LiveView implementation.

This strategy can't possibly succeed. What the library consumers need isn't just to eliminate Surface dependency, but also to eliminate Surface design patterns and mental model as a whole.

[!NOTE] Moon Elixir current design philosophy is oriented around naively mimicking React patterns. This is dangerous as LiveView and React similarities are only superficial. A good enough LiveView design system must be LiveView oriented and mindful of its limitations and strengths.

Current Moon LiveView API and its shortcomings

The formerly proposed LiveView api is problematic due to the dogmatic usage of compound components design:

Basic:

The most common use case requires consumers to write too much markup and duplication.

<.field label="Label for textinput" hint="Hint for Textinput" field={form[:username]}>
  <.input field={form[:username]} />
</.field>

Advanced customization:

If the basic scenario requires too much markup, advanced one is plagued by too much repetition and markup. It is fragile against changes plus it doesn't do a complete job at making customization possible, since the error section is impossible to customize.

<.field field={form[:agrees_to_terms_of_service]}>
  <.label field={form[:agrees_to_terms_of_service]} size="sm">
    <Form.switch field={form[:agrees_to_terms_of_service]} /> I agree terms of service
  </.label>
</.field>

There's no silver buller

Compound Components are currently seen as a silver-bullet and the technique is applied even when unecessary. Any dogmatic approach is superficial and harmful. MoonDS requires actual design principles to succeed, instead of rules to follow blindly.

[!TIP] Compound components are only one possible solution to a specific problem.

Clarifying the problem

The problem solved by this technique is to:

  1. Provide an expressive API which doesn't dictate rendering of children
  2. Manage implicit state in the parent, which is used by children to render themselves
  3. Expess a mandatory relationship between components that only make sense when together

Only part 1. is relevant to form inputs

[!IMPORTANT] A useful guiding principle to design form inputs, and many other components is:

Provide opinionated rendering (default visual design) while enabling rendering customization at the same time.

Alternatives

1) Use :let to forward the child elements assigns

<.switch_field :let={children} field={form[:agrees_to_terms_of_service]}>
  <.label {children.label} size="custom">
    <.switch_input {children.switch_input}/> I agree terms of service
  </.label>
</.switch_field>

A very nice advantage of this technique is hiding implementation details of which assigns label and switch_input require to be wired well with switch_field. Moon is free to change as many of them as desired without incurring breaking changes on consumers, or requiring knowledge about which assigns they are.

It's not my preferred option for the Form Inputs, but it will definitely be useful to other types of components.

2) Use :let to forward the child elements assigns AND child function components

This is but an experimental option. I'm not sure if it would incur perf issues. The technique allows forwarding and entire component, and not only it's assigns.

<.switch_field :let={children} field={form[:agrees_to_terms_of_service]}>
  <.label {children.label} size="custom">
    <%= children.input_element.() %> I agree terms of service
  </.label>
</.switch_field>

About this Document

This document is written like it's meant for a "Request for Comments" (RFC) process. It's a practice from mature engineering organizations to:

Moon could greatly benefit from an RFC process and help the company spread this practice. Read more about React RFC Process or the A Structured RFC Process.

phgrey commented 8 months ago
  1. vanila: the only problem i do see here is additional layer of components {text|switch...}_field with base components API duplicating.
  2. namespace: not sure about Moon.LiveView. I'd like to see live_view components in a separate package/repo to avoid extra dependencies, so would like to have package-based namespace, e.g. MoonView. discussable
gfrancischelli commented 8 months ago
  1. vanila: the only problem i do see here is additional layer of components {text|switch...}_field with base components API duplicating.

I'm not sure if I understood 🤔, which component is the duplicate of switch_field ?

  1. namespace: not sure about Moon.LiveView. I'd like to see live_view components in a separate package/repo to avoid extra dependencies, so would like to have package-based namespace, e.g. MoonView. discussable

A fresh start sounds like a great idea

phgrey commented 8 months ago

switch_field component will have all the attributes from switch_input (now knows as Form.switch). the rendering method itself will be a copy of checkbox_field. the same for input, textarea, select, ... this is how we break DRY here

in general, additional layer of vanila *_field components is hiding some templates complexity inside moon library. but it also can be done on a project-level.

gfrancischelli commented 8 months ago

Oh yeah, that's right. switch_input will receive a subset of switch_field assigns. I don't think that's a problem.

The value of an abstraction resides in:

Thus making switch_field super valuable to regular users

lauaviin commented 8 months ago

In general, I'm content and pleased with the current approach.

Correct me if I'm wrong. Would this proposal change the way we consume all forms of inputs or only the ones wrapped with a form?

Namely, I find it's essential to be mindful of potential disruptions for users heavily reliant on input elements that aren't encapsulated within a form(us in 8io currently). Where we employ a controlled approach where the callsite retains full control over the component's state (we pass the inputs' values via props mostly and the component only serves a representational purpose), rather than relying on the <Form>'s abstraction. We've used the controlled approach due to sometimes requiring special handling from either overrides or where it's less markup to achieve the same results. Perhaps, we can strike a balance and avoid unintended consequences for users who heavily leverage input workflows.

gfrancischelli commented 8 months ago

The proposed pattern can easily accommodate formless inputs:

Usage without forms:

Consumer's just need to pass whatever mandatory values were calculated from the Form.Field struct:

<.switch_field name="activate" value={@value} label="Activate" phx-change={ "my_event" or JS.command() } />

Implementation details:

def switch_field(assigns) do
  assigns = assigns_from_field(assigns)
  ~H"""
  <div phx-feedback-for={@name}> 
    ...
  </div>
  """
end

def assigns_from_field(%{field: %Phoenix.HTML.FormField{} = field} = assigns) do
  # Calculate assigns based on form field
end

# Ignore form otherwise
def assigns_from_field(assigns), do: assigns
lauaviin commented 8 months ago

The proposed pattern can easily accommodate formless inputs:

Perfect! I don't have any objections. I can see it have a positive effect for users who have to work with a lot of forms :muscle: .

gfrancischelli commented 8 months ago

Alternative api name:

Just use regular input

<.input type="switch />