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

Autocomplete: Refactor to use DropdownItem ShowCheckbox feature #5319

Conversation

David-Moreira
Copy link
Contributor

No idea why check handler is not working properly when in AutoComplete. Gotta figure it out.

@David-Moreira David-Moreira changed the title Autocomplete | Refactor to use DropdownItem ShowCheckbox feature WIP | Autocomplete | Refactor to use DropdownItem ShowCheckbox feature Feb 17, 2024
@David-Moreira
Copy link
Contributor Author

Note to self: Investigate whether it's to do with TextFocused

@stsrki
Copy link
Collaborator

stsrki commented Feb 17, 2024

Can it be because of closable.js that is implemented in dropdown?

@David-Moreira
Copy link
Contributor Author

David-Moreira commented Feb 17, 2024

Can it be because of closable.js that is implemented in dropdown?

Can it be because of closable.js that is implemented in dropdown?

Nah, that's does not seem like it.
To re-explain the problem.

  • Clicking the anchor structure is fine.
  • Clicking the structure that makes up the checkbox makes it so it tries to focus the checkbox which in turn will trigger the TextBlur and TextFocused = false from the Autocomplete, which will make the dropdown not visible.
    • Or at least from my investigation, that's what it seems to be happening.

I investigated a while on how to make an element no focusable in html, people say, disabled it the way to go. I just tried to disable the checkbox and it indeed worked... But as you know disabling it, is not really a great option. I suspect I had similar problems before when implementing checkboxes that's why we implemented an icon that acts as a checkbox.

Do you have any suggestions?
I'm thinking in hacking away at it, propagate the Checkbox Focused event, and when the TextBlur event triggers, if it was after the checkbox focused event, we'll ignore it...
But I'm taking a break, I'll think a bit more about it.

@David-Moreira
Copy link
Contributor Author

The Pr's for the checkbox implementation are hard to navigate, but it looks like I found the same limitations at the time.
It seems that at the time, the option we were taking was disabled + styling... hmmm

image

@David-Moreira
Copy link
Contributor Author

Note to self:

  • Fix tests
  • Fix FluentUI look

@David-Moreira
Copy link
Contributor Author

@stsrki I'm not able to compile scss at all. Can you try it?

Also could you try fixing fluent ui checkbox look?
image

Copy link
Collaborator

@stsrki stsrki left a comment

Choose a reason for hiding this comment

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

Did you notice that when you clock on a BS checkbox instead of a dropdown item, that the menu is closed? Can we keep it open because user will expect to keep it while selecting.

@David-Moreira
Copy link
Contributor Author

David-Moreira commented Feb 27, 2024

Did you notice that when you clock on a BS checkbox instead of a dropdown item, that the menu is closed? Can we keep it open because user will expect to keep it while selecting.

Can you give me the steps to reproduce.
It seems to be working correctly.
It is of note, however, that when you unselect the menu will close but this is regardless whether its a checkbox or dropdown click. But that's something that already happens. You could indeed argue this is a bug, and I would fix it in a separate task.

@stsrki
Copy link
Collaborator

stsrki commented Feb 27, 2024

It is of note, however, that when you unselect the menu will close but this is regardless whether its a checkbox or dropdown click.

Yep, this basically. If you could fix this it would be great.

@David-Moreira
Copy link
Contributor Author

It is of note, however, that when you unselect the menu will close but this is regardless whether its a checkbox or dropdown click.

Yep, this basically. If you could fix this it would be great.

Great. Since it's a production bug, I'd proactively fix it as part of v1.4. Going ahead and opening a separate issue. Please review this feature regardless of this particular bug or we can leave it on hold until we merge the fix in.

@stsrki stsrki changed the title WIP | Autocomplete | Refactor to use DropdownItem ShowCheckbox feature Autocomplete: Refactor to use DropdownItem ShowCheckbox feature Feb 27, 2024
@stsrki stsrki merged commit 7ad4e7e into master Feb 27, 2024
2 checks passed
@stsrki stsrki deleted the 4805-autocomplete-checkbox-refactor-to-use-dropdown-item-checkbox-feature branch February 27, 2024 15:39
@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocomplete - Checkbox refactor to use Dropdown Item Checkbox feature
2 participants