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

combobox - select all example #3378

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ThusharaJ07
Copy link
Contributor

@ThusharaJ07 ThusharaJ07 commented May 8, 2024

Implements #3307

Copy link

changeset-bot bot commented May 8, 2024

⚠️ No Changeset found

Latest commit: a05a2f9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented May 8, 2024

@ThusharaJ07 is attempting to deploy a commit to the FED Team Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented May 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
saltdesignsystem ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 21, 2024 8:57am

style={{ width: "266px" }}
>
{filteredOptions.length > 1 && (
<Option
Copy link
Contributor

Choose a reason for hiding this comment

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

Select All should be in it's own which will allow you to remove the styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean by this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry a word got omitted, OptionGroup should be used here

) => {
let newOptionsSelected = [...newSelected];
//case: clear all if select all is unselected
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment above says this check is for is all is unselected but the logic is for if all is selected and it was previously selected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected the comment, it for case where all select all was previously selected, if user unselects any option, then select all should be unselected

Copy link
Contributor

Choose a reason for hiding this comment

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

Whenever we need comments to explain code, it's usually a sign that the code does not explain itself and there is potential to refactor.

As these code are docs, it's worthwhile to consider how we could make the code, explain itself.

  const handleSelectionChange: DropdownProps["onSelectionChange"] = (
    event,
    newSelected
  ) => {
    let newOptionsSelected = [...newSelected];
    const wasAllSelected = selected.includes(allSelectedOptionValue);
    const isAllSelected = newOptionsSelected.includes(allSelectedOptionValue);

    if (wasAllSelected) {
      if (isAllSelected) {
        newOptionsSelected = newOptionsSelected.filter(
          (el) => el !== allSelectedOptionValue
        );
      } else {
        newOptionsSelected = [];
      }
    } else if (
      isAllSelected ||
      (!isAllSelected && newOptionsSelected.length === usStates.length)
    ) {
      newOptionsSelected = [...usStates, allSelectedOptionValue];
    }
    setSelected(newOptionsSelected);
    args.onSelectionChange?.(event, newOptionsSelected);
  };

How about this, any easier to read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes its easier! I have updated this. Thanks Mark for this.

Copy link
Contributor

@origami-z origami-z left a comment

Choose a reason for hiding this comment

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

When i use keyboard to toggle "Select All" option (https://62565e7f.saltdesignsystem-storybook.pages.dev/?path=/story/core-dropdown--select-all), focus is changed to the 2nd item. Is it the intent?

@ThusharaJ07
Copy link
Contributor Author

When i use keyboard to toggle "Select All" option (https://62565e7f.saltdesignsystem-storybook.pages.dev/?path=/story/core-dropdown--select-all), focus is changed to the 2nd item. Is it the intent?

Hey @origami-z , this is an existing issue in dropdown which Shey said will have a word with the team separately. not related to this example.

@origami-z
Copy link
Contributor

Hey @origami-z , this is an existing issue in dropdown which Shey said will have a word with the team separately. not related to this example.

I can only see the issue in dropdown but not combobox, is it to do with key ?

@ThusharaJ07 ThusharaJ07 changed the title added examples for select all in dropdown added examples for select all in combobox May 29, 2024
@ThusharaJ07 ThusharaJ07 changed the title added examples for select all in combobox combobox - select all example May 29, 2024
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.

None yet

4 participants