josefarias / hotwire_combobox

An accessible autocomplete for Ruby on Rails.
https://hotwirecombobox.com
MIT License
436 stars 26 forks source link

"X" icon isn't shown on the handle when selected in mobile dialog #184

Closed searls closed 2 weeks ago

searls commented 1 month ago

I'm having some issues keeping straight the behavioral and visual differences between the mobile dialog and the normal interface.

Most importantly, I'm finding it really hard to figure out how to communicate to users to clear the combobox. In the desktop view, when an option is selected, an "X" is shown because data-queried is added to the input via the _markQueried() method. If the user clicks the X, the selection is cleared and the list is shown. If the user clicks the body, the

screenshot-2024-07-11-09h43m56s

_markQueried() is also called when selected via mobile, however data-queried is set on the wrong input (.hw-combobox__dialog__input instead of .hw-combobox__input), which results in the triangle continuing to be shown after blur:

screenshot-2024-07-11-09h35m08s

searls commented 1 month ago

(Note that this is even more confusing because the arrow acts like an X and will clear the selected option when this occurs)

As a workaround, here is my wrong solution (since doing this via querySelector is probably not how you'd want to do it:

// app/javascript/ext/hotwire_combobox_ext.js
import HwComboboxController from 'controllers/hw_combobox_controller'

HwComboboxController.prototype._markQueried = function () {
  this.element.querySelector('.hw-combobox__input').toggleAttribute('data-queried', this._isQueried)
}
searls commented 2 weeks ago

I've sorta continued to build up a small mountain of hacks to deal with the fact I can't programmatically set values or tell hotwire combobox to update itself.

It got more complicated today because I'm doing a turbo stream idiomorph update to the container, which could result in the value attr of the combobox's input changing, but that change won't update the combobox's appearance (or trigger any other hooks), because the value isn't tracked as a stimulus value.

Here's what I've got now for anyone googling and landing here:

// app/javascript/controllers/hotwire_combobox_wrapper_controller.js
import { Controller } from '@hotwired/stimulus'

// We need to monitor each hotwire combobox and tell it that
// The input changed in order to update the [data-queried] attr on the input.
// Why? Because the presence of that attr will trigger CSS to show the "X" icon
// See: https://github.com/josefarias/hotwire_combobox/issues/184
export default class extends Controller {
  static targets = ['hotwireCombobox', 'hotwireInput']

  connect () {
    this.observeCombobox()
    this.hotwireComboboxTarget.dispatchEvent(new CustomEvent('hotwire-combobox-wrapper:mutated'))
  }

  disconnect () {
    this.disconnectObserver()
  }

  observeCombobox () {
    this.observer = new window.MutationObserver((mutationsList) => {
      mutationsList.forEach((mutation) => {
        if (mutation.type === 'attributes' && (mutation.attributeName === 'value' || mutation.attributeName === 'data-queried')) {
          this.hotwireComboboxTarget.dispatchEvent(new CustomEvent('hotwire-combobox-wrapper:mutated'))
        }
      })
    })

    // Start observing the target for attribute changes
    this.observer.observe(this.hotwireInputTarget, {
      attributes: true,
      childList: false,
      subtree: false
    })
  }

  disconnectObserver () {
    // Disconnect the observer if it exists
    if (this.observer) {
      this.observer.disconnect()
      this.observer = null
    }
  }
}

Separately, in my custom monkey patches:

// app/javascript/ext/hotwire_combobox_ext.js
// Making this as a fake action so I can wire it up on input change
HwComboboxController.prototype.updateHandleIcon = function () {
  this._markQueried()
}

// See: https://github.com/josefarias/hotwire_combobox/issues/184
HwComboboxController.prototype._markQueried = function () {
  this.element.querySelector('.hw-combobox__input').toggleAttribute('data-queried', this._isQueried)
}

I wire this controller up on each relevant combobox itself:

<%= faux_form.combobox "replacement_movement_block_#{block_index}_set_#{set_index}",
        some_search_path,
        input: {
          # String necessary b/c hwcombobox will not translate data_attrs to kebab case
          "data-hotwire-combobox-wrapper-target": "hotwireInput",
        },
        data: {
          controller: "hotwire-combobox-wrapper",
          hotwire_combobox_wrapper_target: "hotwireCombobox",
          action: "hotwire-combobox-wrapper:mutated->hw-combobox#updateHandleIcon",
        } %>
josefarias commented 2 weeks ago

Hey @searls sorry you had to build around the combobox.

I believe https://github.com/josefarias/hotwire_combobox/pull/192 should do what you're looking for. If you don't mind, could you point your gem to main and let me know if it's working for you? I might cut a new release soon, too.

I'm doing a turbo stream idiomorph update to the container, which could result in the value attr of the combobox's input changing, but that change won't update the combobox's appearance (or trigger any other hooks), because the value isn't tracked as a stimulus value.

I'm not sure this is the direction we're headed for what it's worth. But I'm open to changing my mind if there's a compelling use case. I've made the suggestion before to replace the whole combobox. The linked comment isn't your exact situation, but I think that should work for you — just morph away the whole thing (EDIT: assuming that's possible, probably library-dependent, could also replace with a sibling turbo-stream).

Closing for now. If you'd like to discuss the value morphing scenario feel free to open up a discussion. Thanks for reporting!

josefarias commented 2 weeks ago

Ah missed the part where you're using turbo with idiomoprh. I don't think you can morph away the whole thing today without being clever about it. But you can replace with another stream.

I'll think about observing changes to the value. I'm worried it opens the door to too much state juggling.

searls commented 1 week ago

@josefarias I hear you. For what it's worth, I think the state juggling would be worth it because it'd make it harder for the combobox to fall out of sync with the DOM (which is like a ball being dropped now, just not by the library's actions per se).

Personally, I've been extremely pleased with how reliable Stimulus is when an input value is paired with a stimulus value and kept in sync via a pair of data-action="change->#updateSomeValue" from the form and a someValueChanged callback on the controller. There's a lot of things to worry about coordinating in general, but that one has yet to bite me.