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

feat(multi-select, simple-select): add listActionButton & onListAction #7044

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tomdavies73
Copy link
Contributor

fix #6752

Proposed behaviour

Adds the listActionButton prop and onListAction callback to the MultiSelect and SimpleSelect components.

Current behaviour

Currently, only FilterableSelect has the listActionButton prop and onListAction callback.

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

@@ -284,6 +284,65 @@ export const MultiSelectMultiColumnsComponent = (
);
};

export const MultiSelectWithActionButtonComponent = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): might be worth just using the list action button to add the new options so it's a bit simpler, opening a dialog seems a bit left field to me

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave this for now until we've had confirmation about how it should behave when the button is clicked

@@ -365,6 +424,29 @@ export const MultiSelectCustomColorComponent = (
);
};

export const MultiSelectListActionEventComponent = (
props: Partial<MultiSelectProps>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): you could make onListAction Required here so it's flagged if someone ever uses it and doesn't pass the callback etc

@@ -529,6 +547,11 @@ export const MultiSelect = React.forwardRef<HTMLInputElement, MultiSelectProps>(
isMouseDownReported.current = true;
}

function handleOnListAction() {
setOpenState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: as the list closes when the button is clicked, will this really work for lazy loading?

@harpalsingh fyi

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

Successfully merging this pull request may close these issues.

MutiSelect: add onListScrollBottom prop.
3 participants