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

Selection in one super select affects options of another. #42

Closed
andrewculver opened this issue Nov 29, 2022 · 11 comments · Fixed by bullet-train-co/bullet_train-core#534 · May be fixed by #45
Closed

Selection in one super select affects options of another. #42

andrewculver opened this issue Nov 29, 2022 · 11 comments · Fixed by bullet-train-co/bullet_train-core#534 · May be fixed by #45
Assignees

Comments

@andrewculver
Copy link
Contributor

Background

Hypothetically, when setting up a webhooks endpoint for scaffolding_completely_concrete_tangible_things.updated, we want users to be able to specify that they only want to receive a webhook when a specific tangible thing is updated. The current plan is to implement this by adding a scaffolding_completely_concrete_tangible_thing_id attribute to Webhooks::Outgoing::Endpoint. (We're also going to add a scaffolding_absolutely_abstract_creative_concept_id attribute as well, for filtering those kinds of events.)

Assume there is both a "Creative Concept" and "Tangible Thing" super select field on the form for Webhooks::Outgoing::Endpoint.

Request

  1. Before a Creative Concept is selected, I think the Tangible Things field should be disabled by default.
  2. When someone selects a Creative Concept, I want the list of options in the Tangible Things select field to only be those options that exist under the selected Creative Concept.
  3. I'd like this to be implemented in a way where it could be used multiple levels deep, e.g. Country > Region > City.

Initial Thoughts

I think this should be relatively straightforward, since we have the ability to populate a Super Select based on an AJAX request and you can hit /account/scaffolding/absolutely_abstract/creative_concepts/$CREATIVE_CONCEPT_ID/completely_concrete/tangible_things.json and get a list of Tangible Things for a given Creative Concept.

@andrewculver
Copy link
Contributor Author

@pascallaliberte It's like we need to be able to piggy-back on the AJAX feature and just specify that the $CREATIVE_CONCEPT_ID part of the URL comes from "that other field" and this field should be disabled unless that value is selected.

@pascallaliberte
Copy link
Member

@andrewculver How likely is it that the field label will change in the Webhook#new form?

e.g. Let's take the Country > Region example. Instead of "Region", if you select "Canada" as the country, the region label will be "Province" and if United States, will be "State".

In that case, a turbo_frame will be what's called for.

@andrewculver
Copy link
Contributor Author

I think it’s quite rare. The only example where I can think of that being needed is an address field. I probably wouldn’t solve for that specific problem necessarily. In particular, we actually want to introduce a package specifically for supporting address fields, so we could sell for that separately.

@pascallaliberte
Copy link
Member

@andrewculver made some headway into this.

I've chosen to equip super-select to update its own searchUrl value from the values of fields on which it depends, and for that, I introduced a new dependable_controller, re-usable for other uses.

I chose not to use the new outlets API from Stimulus 3.2, because it's meant for tight-coupling situations.

@pascallaliberte
Copy link
Member

@andrewculver

Two problems I've run across, making super-select not a first-party player to our api endpoints:

  1. super-select's ajax functionality isn't made to deduce the items from a json call to one of our endpoints. It assumes the json endpoint returns a .results array with id and text keys, whereas our endpoints are standardized on a root array of items with id and name keys (although that last one is not always true).
  2. the search: true setting, which adds an autocomplete text field, when paired with a choices_url to use ajax, assumes that the results will filter via a search query string (which by default our api endpoints don't act on).

Changing super-select to fix 1. would introduce a breaking change for those who followed select2's own guidance that it "excepts results from the remote endpoint to be filtered on the server-side", and seeing in its examples that it assumes a .results key in the json response and creating one for that purpose. We would therefore need a flag to make super-select opt into using default endpoints. Or maybe the partial could deduce that we're hitting a vanilla api endpoint either by noticing /account/ in the url, or /api/v1/, and then having the smarts to assume a standard response layout.

Then there's the issue of name being the default key instead of text, and a way to override that.

Speaking of /api/v1/, super-select won't be passing the authentication token by default, so I'm assuming we're passing a path like account_country_regions_path(@team.countries.first, format: :json).

For problem 2., we could standardize the search: param across our json endpoints for filtering.

Overall, however, the other alternative is to turbo_frame the whole field and not using select2's ajax capabilities. There's a way to do that, even via super-scaffolding, to make the turbo_frame the current form's URL with ?country_id=<country_id> param in the URL and serving up the right region if available.

Which direction are you leaning toward?

@pascallaliberte
Copy link
Member

It kinda feels like we should go all-in with super-select working with our endpoints out of the box by default, including default support for search query-string param on json endpoints with STARTS_WITH matching at least.

@pascallaliberte
Copy link
Member

@andrewculver Thought some more about this. I see two big approaches:

  1. Go all-in with format.json account-prefixed endpoints. Either break backward compatibility of super-selects assuming a sub-array or add a new partial parameter called from_resource_path which would presume an account-prefixed json endpoint and its response to be formatted correctly. Add a search scope to all controllers when ?search= is in the query string params, applied to both json and html responses.
  2. Scaffold controllers to handle a ?<primarySuperSelectName>= query string param on the :new, :edit actions so that we can update a turbo_frame containing that dependentSuperSelect, and the turbo_frame will just update with the new super select without submitting the form.

Option 1. is better if we ever assume we should handle large sets of values for the super-select.

Option 2. is better if we assume short lists of values. It's also better if there's something else we want updated based on the value selected in the primary super select (e.g. the label of the field, an updated helper text, anything outside of the super-select itself) because the whole turbo_frame will be updated.

@pascallaliberte
Copy link
Member

pascallaliberte commented Jan 27, 2023

Our work on bullet-train-co/bullet_train#635 will solve all of this. It'll update a dependent super select. It'll side-track the problems with searches and ajax response. It'll also avoid touching the controllers to make parts of the form change to a query_string param of the field we're depending on as we update a turbo_frame to include the new super select.

But there is one thing that'll need a little documentation:

  1. I'd like this to be implemented in a way where it could be used multiple levels deep, e.g. Country > Region > City.

Let's wait until 635 is merged and we can look into that piece of documentation.

@pascallaliberte
Copy link
Member

bullet-train-co/bullet_train#635 is now merged. The underpinnings are available for this functionality (and used out-of-the-box in the new Address field implementation). The next step it to document this trick.

Let's keep this issue open to cover the documentation need.

@jagthedrummer
Copy link

@pascallaliberte, do you think we should make an issue in one of the active repos that links to this issue, just so we don't lose track of it? (I had no idea that there were still active issues on repos left over from before The Great Bundling.)

@pascallaliberte
Copy link
Member

@jagthedrummer Closing bullet-train-co/bullet_train-core#534 will close this one. It's also referenced in issue bullet-train-co/bullet_train-core#533

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment