ni / nimble

The NI Nimble Design System
https://nimble.ni.dev
MIT License
29 stars 8 forks source link

Align combobox implementation behavior with select. #1948

Open atmgrifter00 opened 4 months ago

atmgrifter00 commented 4 months ago

🧹 Tech Debt

The initial attempt to align the Combobox implementation with the Select resulted in a bug in Angular, where the options would ultimately remain hidden upon a reload of the DOM after they had been filtered out. The cause of this seems rooted in the new registerOption implementation, but it's unclear at the moment exactly why this is resulting in the new behavior.

Ultimately, as part of re-aligning the Combobox and Select implementations, the Combobox should adjust its implementation to no longer manipulate the hidden attribute directly while filtering, and instead leverage the now available visuallyHidden attribute of the ListOption.

atmgrifter00 commented 4 months ago

Ideally, as part of the work to re-align the Combobox with the Select we would also provide a test case that the linked AzDO bug above is representative of. This may be a bit tricky because it involves a re-loading of an Angular component that was hosting the Combobox.

jattasNI commented 3 months ago

@atmgrifter00 we are trying to figure out how to prioritize this tech debt but I think it contains several issues that might have differing priorities. Could you take a pass at listing those and bucketing them into separate issues if we think we should fix them separately?

Brainstormed possible issues with Milan:

jattasNI commented 2 weeks ago

Here's some additional tech and/or architecture debt that might be covered by this.

  1. coupled responsibilities of FAST foundation Listbox base class and Nimble concrete derived classes. See #2058
  2. better isolation of related component state. For example, should we have a separate keyboard manager and selection manager like we do for other components?
  3. ensure synchronous state management to match the native select behavior (or be very clear about when we diverge from this). See #1915. Matching native behavior is important because the Angular CVA assumes we do.

Re-adding the triage tag to trigger a discussion about whether these should be separate issues and what their priority is. Hopefully we can prioritize at least some of it because we had multiple instances of select changes unexpectedly breaking client apps and needing to be reverted and we believe these issues contributed to that happening.

jattasNI commented 1 week ago

@atmgrifter00 and the team agreed that he'll work on these issues this cycle and come up with a recommendation of how to split up the work once he's done some initial research. This will be be prioritized after the first 1 or 2 SLE combobox -> select migrations.