这是indexloc提供的服务，不要输入任何密码
Skip to content

Allow deselect when renderSelectedChoices enabled #1221

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow deselect when renderSelectedChoices enabled #1221

wants to merge 1 commit into from

Conversation

godismyjudge95
Copy link

@godismyjudge95 godismyjudge95 commented Oct 11, 2024 •
edited
Loading

Description

Drafting this PR as I am not sure if you would want to follow a different approach. If this looks good I can try to add tests.

There have been a few requests to allow for deselecting items via the dropdown when the renderSelectedChoices option is enabled. #804 #884

I ran across this issue myself while needing to keep a multiselect box footprint minimal (it grows to be pretty large when selecting a few options).

This PR has 3 parts:m

  • highlight selected choices in the dropdown when renderSelectedChoices is enabled
  • allow clicking on a selected choice in the dropdown to deselect the choice
  • add a new renderItems boolean option to disable the "pills" in the multiselect input to allow one to only select/deselect via the dropdown

Screenshots (if appropriate)

Example of having both renderSelectedChoices: 'always' and renderItems: false

image

Types of changes

  • Chore (tooling change or documentation change)
  • Refactor (non-breaking change which maintains existing functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • My code follows the code style of this project.
  • I have added new tests for the bug I fixed/the new feature I added.
  • I have modified existing tests for the bug I fixed/the new feature I added.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@Xon Xon self-requested a review October 25, 2024 03:08
Xon
Xon reviewed Oct 25, 2024
View reviewed changes
Xon
Xon reviewed Oct 25, 2024
View reviewed changes
Xon
Xon reviewed Oct 25, 2024
View reviewed changes
Xon
Xon requested changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@Xon Xon left a comment

Choose a reason for hiding this comment

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

Please ensure unit tests and e2e tests are passing.

  • An e2e test for each functionality change should also be added. Ideally one each for text, select-one and select-multiple even if it is testing it doesn't do anything.

This change has a lot of unrelated code churn which complicates git blame. Please rebuild the PR without those changes to preserve git blame

Changes to .js files not matched in the .ts files. The .ts are the canonical source and the .js changes will be discard at the next build. There are significant changes not committed into the .ts

@Xon Xon added feature Pull request that adds new functionality changes required Pull request requires changes before it can be merged labels Oct 25, 2024
@igorzharov
Copy link

Когда уже сделайте?!)

@godismyjudge95
Copy link
Author

Please ensure unit tests and e2e tests are passing.

All tests except the demo-page screenshot test are passing. I am not sure how to resolve that one? Do I need to generate a new screenshot?

  • An e2e test for each functionality change should also be added. Ideally one each for text, select-one and select-multiple even if it is testing it doesn't do anything.

I added a test for select-one and select-multiple. Not sure the text one is relevant in this case?
Select one test ensures it does not show the deselect text as this PR does not enable that - although it could.

This change has a lot of unrelated code churn which complicates git blame. Please rebuild the PR without those changes to preserve git blame

Apologies, this PR was rushed just to see if you'd be open to the idea. The PR should now be much more concise.

Changes to .js files not matched in the .ts files. The .ts are the canonical source and the .js changes will be discard at the next build. There are significant changes not committed into the .ts

This is fixed now.

@godismyjudge95 godismyjudge95 marked this pull request as ready for review May 12, 2025 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xon Xon Xon requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
changes required Pull request requires changes before it can be merged feature Pull request that adds new functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@godismyjudge95 @igorzharov @Xon