search

zinc-collective / convene

An Operating System for the Solidarity Economy
https://convene.zinc.coop
Other
56 stars 19 forks source link

Removes blank option from Delivery Areas dropdown on Marketplace #1920

Closed rosschapman closed 6 months ago

rosschapman commented 6 months ago

UPDATE 11/7/23: See discussions below for decision history on this issue, including copy updates. In brief, the select partial has been replaced by a new `SelectComponent. Also I have spot tested the application and dropdown selection seems to be working as expected.

Not sure if we meant to add a prompt or not?

image

anaulin commented 6 months ago

Yeah, I noticed this too. I think the ideal solution here would be:

  1. If there is only 1 delivery area option, don't show the dropdown at all. Instead, set the delivery area for the order automatically (e.g. maybe make it a hidden field with the one area id).
  2. If there is more than one delivery area, show the dropdown with a prompt.

Since adding the "default to the area if there is only 1" is probably much bigger of a change that you were planning for, an intermediate solution could be: if there is more than 1 option, show the prompt (maybe "-- Select delivery area --"), and if there is only 1 option, just show that one with one prompt (so that it will be selected by default in the dropdown).

rosschapman commented 6 months ago

If you're feeling extra, see my previous comment on having different behaviors when we have 1 option versus multiple.

@anaulin I'm feeling extra for this one. I think your suggestion makes sense. What do you think about standardizing this behavior in our select partial? Even more, refactoring to a View Component?

zspencer commented 6 months ago

In another codebase; I've played around with using a custom form builder rather than ViewComponents or Partials for defining how forms work. The advantage of that is:

The disadvantages are

rosschapman commented 6 months ago

I don't know how well ViewComponent works when embedded within the FormBuilder, which is how I would prefer we write our more complex UI elements...

I'll take some time to play with nesting a View Component in a form_with block. In my mental model VCs are just partials on a syntactic sugar high, and passing through a form object should work similarly/unsurprisingly?

rosschapman commented 6 months ago

@anaulin @zspencer the latest code adds a new SelectComponent to experiment with View Components nested in a FormBuilder helper. I also experimented with how to display the DeliveryArea to the shopper if the there is only a single DeliveryArea associated to the Marketplace. At first I was considering the select tag wrapper component switch on choices size, but it seemed like a better approach to handle this at a higher level and rethink the UX a bit. If we know ahead of time there's only one option then (rhetorically) is user selection necessary?

image

zspencer commented 6 months ago

I think it's fine to start with a ViewComponent; but where we want to go is to get away from needing people to go "off the rails" so-to-speak when making forms (either by instantiating a ViewComponent or using a render partial: form/{field}.)

The way to do that is to write a Custom Marketplace::FormBuilder and overload the select method definition. It's entirely possible the Marketplace::FormBuilder#select method body will literally be render SelectComponent.new(...); since I think it's fine to use a ViewComponent for the html-building side of things.

Then, we can define a form_with helper method (for now, isolated to the Marketplace, perhaps on the Marketplace::Controller) which inserts the Marketplace::FormBuilder for the builder argument.

That way whenever we want to make forms in the Marketplace (and eventually throughout Convene) we can do the Rails-default thing of form_with.

It may make more sense to pull it out of the Marketplace namespace; but I figure keeping it tightly scoped initially; and then promoting it to the top-level once we want to use it elsewhere would be the safest, smallest step forward.

TL/DR: I would love it if we could ditch render partial: "form/select" and render SelectComponent from our templates; so that we stay "on the Rails".

rosschapman commented 6 months ago

TL/DR: I would love it if we could ditch render partial: "form/select" and render SelectComponent from our templates; so that we stay "on the Rails".

@zspencer Oh, custom form builders. That makes sense. Form libraries scare me coming from the JS ecosystem where there are too many choices and patterns. Also, it's almost rare that reuse works in the long run. For example, lets use this shared form code for both "edit" and "create" forms -- cue eventual cumbersome refactor! This is why I'll always build up base form components first before stuffing them in a general solution.

That said, FormBuilder is intrinsic to Rails so it's comforting to be able to build on top of that abstraction with confidence it's the blessed path.

rosschapman commented 6 months ago

Just discovered a bug because we rely on physically selecting a delivery area to apply the delivery area to the cart. My new UX has far reaching implications! Considering the new design, I will have to figure out a way to set the delivery area for carts when loading the page.

rosschapman commented 6 months ago

@zspencer and I have figured out a path forward for creating the cart with a delivery area if only a single delivery area is associated to a marketplace: https://github.com/zinc-collective/convene/pull/1920/commits/629b53a0252d09af669159ee107b74b36c19e887.

rosschapman commented 6 months ago

Btw @zspencer if the current state of this changeset suffices, great. I've also got a fast follow-up with some view specs for the new conditional logic here