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

[select] popoverTargetProps and inputProps fixes & docs #5730

Merged
merged 4 commits into from
Nov 7, 2022

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Nov 7, 2022

Changes proposed in this pull request:

  • Move popoverTargetProps (prop available on Select2 and MultiSelect2) typedef & docs into SelectPopoverProps
    • Note that popoverTargetProps is not supported on Suggest2, and users should use inputProps instead, as it does the same thing.
  • fix(MultiSelect2): invoke popoverTargetProps onKeyDown/oKeyUp event handlers if provided
  • chore(Suggest2): clean up some code to make event handlers more legible
  • fix(Suggest2): fix support for inputProps.className (it no longer overrides the popover class name, which caused all sorts of things to break)

Reviewers should focus on:

No regressions (should be covered by test suites)

@blueprint-bot
Copy link

[select] fix: unify & clarify popoverTargetProps support

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

fix up various docs & typedefs

Previews: documentation | landing | table | demo

case "keydown":
return event => {
handler?.(event);
this.props.popoverTargetProps?.onKeyDown?.(event);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a bugfix -- popoverTargetProps.onKey{Down|Up} handlers were never getting called before.

@@ -252,6 +254,7 @@ export class Suggest2<T> extends AbstractPureComponent2<Suggest2Props<T>, Sugges
{...inputProps}
aria-autocomplete="list"
aria-expanded={isOpen}
className={classNames(targetProps.className, inputProps.className)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is also a bugfix -- inputProps.className was clobbering targetProps.className before, which would totally break things.

@adidahiya adidahiya changed the title [select] fix: unify & clarify popoverTargetProps support [select] popoverTargetProps and inputProps fixes & docs improvements Nov 7, 2022
@adidahiya adidahiya changed the title [select] popoverTargetProps and inputProps fixes & docs improvements [select] popoverTargetProps and inputProps fixes & docs Nov 7, 2022
@blueprint-bot
Copy link

fix test compilation

Previews: documentation | landing | table | demo

@adidahiya adidahiya merged commit 5003018 into develop Nov 7, 2022
@adidahiya adidahiya deleted the ad/fix-popover-target-props branch November 7, 2022 20:58
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.

2 participants