Skip to content

Conversation

tyb-talks
Copy link
Contributor

@tyb-talks tyb-talks commented Sep 23, 2025

This replaces the jquery-based autocomplete in user-selector with the DMultiSelect component (similar to what was done in discourse/discourse#34685).

Existing behaviour:

text-field-user-selector.mov

Caveat:
I'm unsure how the higher level wizard components depend on the user-selector being a TextField-based component.. If we can't use DMultiSelect here, the alternative would be applying the DAutocomplete modifier directly to the text field (example: discourse/discourse#34131).

Key Changes

  • Converted custom-user-selector from Classic Ember component to Glimmer component, passing in required properties explicitly from parent Classic component

  • Replaced jQuery autocomplete with DMultiSelect for modern UI patterns

  • Maintained backward compatibility with existing @onchangecallback interface

  • Automatic user exclusion: DMultiSelect automatically filters selected users from search results (eliminates
    custom excludedUsernames() logic)

  • Built-in keyboard navigation and accessibility features

  • Search results display: Users show avatars + username/name, groups show group icon + name

  • Selected items display: Plain text (username for users, name for groups) without icons - matches original
    behavior

  • Modern dropdown positioning using FloatKit with configurable placement options

  • Removed Legacy Options

    • allowAny: Not needed with DMultiSelect (was jQuery autocomplete-specific)
    • updateData: Replaced by DMultiSelect's automatic reactive updates
    • Template functions: Replaced with Handlebars named blocks (<:selection>, <:result>)

Testing

New behaviour:

d-multi-select-user-selector.mov

@tyb-talks tyb-talks force-pushed the dev-refactor-custom-user-selector-autocomplete branch from be02794 to e6e6286 Compare September 23, 2025 10:56

actions: {
updateFieldValue(usernames) {
this.set("field.value", usernames);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is meant to do the same thing as the previous 2-way binding from TextField and assumes there's some kind of dependency on this.field.value for this wrapper component. I may be wrong here and this is not necessary.

Copy link
Member

@angusmcleod angusmcleod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyb-talks Amazing, thank you.

@tyb-talks
Copy link
Contributor Author

@tyb-talks Amazing, thank you.

🙇

Could I also trouble you to trigger the CI? (I don't see an option for forcing a run and it seems stuck)

And I don't really have the context to do a comprehensive manual QA on the wizard, if you have something scaffolded up on a test site or locally, it would be safer to do a check on your end. The plugin's test suite fails on a few tests locally which seem unrelated to this change so I suspect my local setup isn't fully compatible with this plugin.

@tyb-talks
Copy link
Contributor Author

@tyb-talks Amazing, thank you.

🙇

Could I also trouble you to trigger the CI? (I don't see an option for forcing a run and it seems stuck)

And I don't really have the context to do a comprehensive manual QA on the wizard, if you have something scaffolded up on a test site or locally, it would be safer to do a check on your end. The plugin's test suite fails on a few tests locally which seem unrelated to this change so I suspect my local setup isn't fully compatible with this plugin.

@angusmcleod in case you missed the above :)

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 this pull request may close these issues.

2 participants