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] Only activate clear button plugin when there is a placeholder option #2282

Open
wants to merge 2 commits into
base: 2.x
Choose a base branch
from

Conversation

Qronicle
Copy link

Q A
Bug fix? yes
New feature? maybe?
Issues Fix #402
License MIT

Only add the clear_button plugin to TomSelect if the form element has no placeholder option.

Notes:

  • I defined 'having a placeholder' as the select having an option with an empty value, as that is what TomSelect also seems to do
  • I'm not 100% sure about whether this needs a test or how to even run tests locally

Ruud Seberechts added 2 commits October 18, 2024 18:09
@carsonbot carsonbot added Autocomplete Bug Bug Fix Status: Needs Review Needs to be reviewed labels Oct 18, 2024
Comment on lines +230 to +231
const placeholderOptions = Array.from(this.selectElement.options)
.filter((option) => option.value === '');
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use Array.from(...).find() here ?
(especially as the placeholder will be first element in most of the cases)

Copy link
Member

Choose a reason for hiding this comment

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

Even .some() that will return you the boolean you need L232 ?

Copy link
Member

Choose a reason for hiding this comment

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

(yeah.. in the .ts file 😅 )

@smnandre
Copy link
Member

Form the monorepository root

Install dependencies

yarn install 

Code style

yarn run lint
yarn run format

Run Tests

yarn run test

Build dist packages

yarn run test

(you need to commit both src/Autocomplete/assets/src/** and src/Autocomplete/assets/dist/** files in this PR)

Please ask if you want/need anymore information :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Autocomplete] required choice types end up not being required.
3 participants