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

Refactor combobox and select for readability, code sharing, and API consistency #1948

Open
11 tasks
atmgrifter00 opened this issue Mar 20, 2024 · 7 comments
Open
11 tasks
Assignees

Comments

@atmgrifter00
Copy link
Contributor

atmgrifter00 commented Mar 20, 2024

🧹 Tech Debt

After recent work on these components we feel that the codebase isn't as easy to work in as we would like and there are opportunities to improve API consistency. We should focus on improving this. As a side effect (but not the primary goal) this could also include:

Tasks

  1. tech debt
  2. tech debt
  3. tech debt
  4. enhancement
  5. enhancement
  6. tech debt
  1. 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.
@atmgrifter00
Copy link
Contributor Author

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.

rajsite pushed a commit that referenced this issue Mar 20, 2024
# Pull Request

## 🤨 Rationale

[AzDO bug 'Nimble combo box | Issue in showing list
options'](https://ni.visualstudio.com/DevCentral/_workitems/edit/2687002)

## 👩‍💻 Implementation

Simple reversion of problem code. I've also created a [tech debt
item](#1948) to track the work needed
to ultimately re-align the `Combobox` and `Select` implementations.

## 🧪 Testing

Just removing the related test.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Mar 26, 2024
rajsite added a commit that referenced this issue Apr 4, 2024
# Pull Request

## 🤨 Rationale

- #1948 

As part of the effort to bring the `Combobox` implementation in
alignment with the `Select`, namely with how we handle controlling the
visibility of the options in the dropdown, as well as the need to
register options as part of the `connectedCallback` hook, it makes sense
to fork the FAST `Combobox` implementation into our own, as we did with
the `Select`, to make it more clear what is happening within our own
implementation for the various new behaviors we require.

## 👩‍💻 Implementation

Essentially bringing over the current `Combobox` implementation from the
FAST `archives/fast-element1` branch into our own. Copied the FAST
`FormAssociatedCombobox` that our `Combobox` now extends.

## 🧪 Testing

Ran existing unit tests. Manual testing. Verified that our existing
tests still replicate all of the tests currently in the FAST
`archives/fast-element1` repo.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Milan Raj <rajsite@users.noreply.github.com>
@jattasNI
Copy link
Contributor

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

  • registerOption
  • visuallyHidden to track filtered items
  • diacritic insensitive search

@jattasNI jattasNI assigned jattasNI and atmgrifter00 and unassigned jattasNI Apr 22, 2024
@jattasNI
Copy link
Contributor

jattasNI commented Jul 1, 2024

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 Sever dependency on FAST Listbox from Nimble Select/Combobox #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 Nimble dropdown components don't match native select option behavior #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 jattasNI added the triage New issue that needs to be reviewed label Jul 1, 2024
@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Jul 9, 2024
@jattasNI
Copy link
Contributor

jattasNI commented Jul 9, 2024

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

@rajsite
Copy link
Member

rajsite commented Jul 23, 2024

Also need to consider dynamic options equivalents for combobox (i.e. combobox analog to select's filter-mode manual)

@jattasNI jattasNI changed the title Align combobox implementation behavior with select. Refactor combobox and select for readability, code sharing, and API consistency Jul 23, 2024
@jattasNI
Copy link
Contributor

With Meyer's help I repurposed this issue to be the grab-bag tech debt issue and linked to all the sub-tasks that might be part of it.

@rajsite
Copy link
Member

rajsite commented Jul 25, 2024

Another item I'd like to see in the alignment design (and might be a bigger set of trade-offs to discuss). It would be "ideal" to have some technical outcomes:

  • We did not rely on light dom id associations. Let users completely own ids, also meaning they could do bad things like have duplicate ids on the page. We won't care because we don't rely on ids
  • Have the aria presenting target (element that announces the role) be in the shadow root. Currently it's inconsistent, the combobox has an input in the shadow root that is the main aria presentation when the select host element is the aria presentation.
  • Drop the element internals polyfill support that has us creating an input element in the shadow root and switch to using element internals directly: https://developer.mozilla.org/en-US/docs/Web/API/ElementInternals
  • Switch to popover api for placing an element in the top layer and to calculate positioning find an anchor positioning polyfill / use a library like floating ui.
  • Using the native picker for mobile platforms for selecting an option instead of the rendered popup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Current Iteration
Development

No branches or pull requests

4 participants