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

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

Closed
searls opened this issue Jul 11, 2024 · 5 comments · Fixed by #192
Closed

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

searls opened this issue Jul 11, 2024 · 5 comments · Fixed by #192

Comments

@searls
Copy link
Contributor

searls commented Jul 11, 2024

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 searls changed the title "X" icon isn't shown on the handle when selected in mobile dialog for async comboboxes "X" icon isn't shown on the handle when selected in mobile dialog Jul 11, 2024
@searls
Copy link
Contributor Author

searls commented Jul 11, 2024

(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
Copy link
Contributor Author

searls commented Jul 29, 2024

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
Copy link
Owner

josefarias commented Jul 30, 2024

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

I believe #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
Copy link
Owner

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
Copy link
Contributor Author

searls commented Aug 4, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants