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

Add @searchFieldPosition argument #1784

Closed

Conversation

alex-ju
Copy link
Contributor

@alex-ju alex-ju commented Apr 16, 2024

Summary

Follow-up on #1752, gradually introducing @searchFieldPosition (after-options | trigger).

Description

<PowerSelect /> instances will have @searchFieldPosition set to before-options by default.
<PowerSelectMultiple /> instances will have @searchFieldPosition set to trigger by default with the option to change it to before-options.

<PowerSelectMultiple @searchEnabled={{true}} @searchFieldPosition="before-options" /> instances will have a search input placed in the dropdown, before the options, acting exactly as it currently does in <PowerSelect @searchEnabled={{true}} />.

`<PowerSelect />` instances will have `@searchFieldPosition` set to `before-options` by default. `<PowerSelectMultiple />` instances will have `@searchFieldPosition` set to `trigger` by default with the option to change it to `before-options`.
@alex-ju alex-ju marked this pull request as ready for review April 16, 2024 15:25
@mkszepp
Copy link
Collaborator

mkszepp commented Apr 18, 2024

@alex-ju thank you... i will look to find time in next days to review it

@mkszepp
Copy link
Collaborator

mkszepp commented Apr 22, 2024

@alex-ju i have tested passing @searchFieldPosition for multiple and single select.
Multiple works great, but the @searchFieldPosition="trigger" on single doesn't work atm.

Do you have time to add also that option for single select?

@alex-ju
Copy link
Contributor Author

alex-ju commented Apr 22, 2024

@alex-ju i have tested passing @searchFieldPosition for multiple and single select. Multiple works great, but the @searchFieldPosition="trigger" on single doesn't work atm.

Do you have time to add also that option for single select?

I had a start on the single select as well, but it requires a bit more attention (esp on the a11y and focus management) so I was hoping to address it in a follow-up PR.

@mkszepp
Copy link
Collaborator

mkszepp commented Apr 22, 2024

I had a start on the single select as well, but it requires a bit more attention (esp on the a11y and focus management) so I was hoping to address it in a follow-up PR.

ah, understood...
i would like to add in same PR, because we need to add the new paramenter in docs, so maybe other peoples are reporting us that the new paramenter is not working.

@alex-ju
Copy link
Contributor Author

alex-ju commented Apr 22, 2024

I had a start on the single select as well, but it requires a bit more attention (esp on the a11y and focus management) so I was hoping to address it in a follow-up PR.

ah, understood... i would like to add in same PR, because we need to add the new paramenter in docs, so maybe other peoples are reporting us that the new paramenter is not working.

makes sense, will try to carry on with this PR then when time allows

@alex-ju
Copy link
Contributor Author

alex-ju commented Jul 1, 2024

@mkszepp an accessible implementation of @searchFieldPosition="trigger" on single selection turns out to be rather complex. would you consider gradually introducing all combinations, by first getting @searchFieldPosition="after-options" for multiple selection?

@MelSumner
Copy link

👋 I'd love to have this accessibility improvement go live. Any ideas on how we can move this PR forward? TIA for considering! <3

@mkszepp
Copy link
Collaborator

mkszepp commented Nov 27, 2024

@MelSumner atm i'm a little bit busy, but i would like to bring this feature to finish line. I think the open task is to bring the same logic like @alex-ju did already for multiple, to single select... (i think handling a11y support correctly, was a blocking point for him)

I would like to introduce this feature in current major version (v8) and before we make braking changes, because for sass we need to do a braking change (moving to modern api, replace @import with @use and drop support for non dart-sass). This is planned for begin of next year, otherwise we block peoples for using modern api (in my app, we were moved to vanilla js to fix this issue for now)

Would you like to complete this feature? or @alex-ju do you have any time to bring this to finish line?

Otherwise let me know, so i will look to schedule time for this feature.

@alex-ju
Copy link
Contributor Author

alex-ju commented Dec 2, 2024

@MelSumner atm i'm a little bit busy, but i would like to bring this feature to finish line. I think the open task is to bring the same logic like @alex-ju did already for multiple, to single select... (i think handling a11y support correctly, was a blocking point for him)

@searchFieldPosition="after-options" introduced in this PR for the multi-select is already available for the single-select. this is the accessible, compliant option @MelSumner and I were looking forward to.

@searchFieldPosition="trigger" is currently only available for the multi-select. Adding trigger support to the single-select is not trivial for an accessible, compliant implementation – hence, I suggest introducing such features gradually rather than all at once.

I would like to introduce this feature in current major version (v8) and before we make braking changes, because for sass we need to do a braking change (moving to modern api, replace @import with @use and drop support for non dart-sass). This is planned for begin of next year, otherwise we block peoples for using modern api (in my app, we were moved to vanilla js to fix this issue for now)

This sounds great!

Would you like to complete this feature? or @alex-ju do you have any time to bring this to finish line?
Otherwise let me know, so i will look to schedule time for this feature.

If you think trigger support for the single-select is a must I will have to defer this to you, I'm afraid. Feel free to carry on with this PR or raise a separate one.

@mkszepp
Copy link
Collaborator

mkszepp commented Dec 8, 2024

i have taken the changes from this PR and added the same for single select field in PR #1864
Tests should be pass (when npm registry is again fixed and up... npm is down :( )

i will close this PR as we have everything in PR #1864

@alex-ju thank you again for your input & help for adding this feature

@mkszepp mkszepp closed this Dec 8, 2024
@alex-ju alex-ju deleted the alex-ju/add-search-field-position branch December 9, 2024 08:34
@mkszepp
Copy link
Collaborator

mkszepp commented Dec 11, 2024

@alex-ju @MelSumner released in v8.6.0

@alex-ju
Copy link
Contributor Author

alex-ju commented Dec 11, 2024

@alex-ju @MelSumner released in v8.6.0

@mkszepp awesome, thanks for getting this over the line 🙌

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.

3 participants