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

[Labs] Pass inputRef to <Suggest> & <MultiSelect> #2079

Closed
wants to merge 1 commit into from

Conversation

alxmiron
Copy link
Contributor

@alxmiron alxmiron commented Feb 3, 2018

Changes proposed in this pull request:

  1. Pass inputProps.inputRef prop to Suggest component, in same way as in Select.
  2. Pass tagInputProps.inputProps.ref to MultiSelect component. (inputProps is passed to TagInput, that does not accept inputRef, because uses native input tag instead of InputGroup)

Passing custom ref handler to input will allow more flexible opportunity to build own extended components on top of blueprint components.

this.input = ref;

const { tagInputProps = {} } = this.props;
const inputProps: HTMLInputProps = tagInputProps.inputProps || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

const { tagInput: { inputProps = {} } } = this.props should do the trick

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if tagInputProps is undefined, will this line brake?

Copy link
Contributor

Choose a reason for hiding this comment

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

truth, it will. then do the same thing twice.
= {} is the new || {} (and so much more typesafe)

const { tagInputProps = {} } = this.props;
const { inputProps = {} } = tagInputProps;

const { tagInputProps = {} } = this.props;
const inputProps: HTMLInputProps = tagInputProps.inputProps || {};
// can't use safeInvoke cuz inputProps.ref can be `string | function`
const refHandler = inputProps.ref;
Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexMarvelo why does this file use .ref while the one below uses .inputRef?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Described in PR description, point 2

Copy link
Contributor

Choose a reason for hiding this comment

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

this PR for 2.0 added TagInput.inputRef prop: https://github.com/palantir/blueprint/pull/2034/files#diff-534314697ccd64472fc61bbacae83ae1R34

i'm inclined to wait for 2.0 for this whole thing. is that acceptable?

@alxmiron
Copy link
Contributor Author

alxmiron commented Feb 6, 2018

Are there any problems here?

@giladgray
Copy link
Contributor

sorry @AlexMarvelo i'm fully focused on getting 2.0 out the door this week, so this PR is low priority. left some comments but I've got my plate quite full.

@giladgray
Copy link
Contributor

@AlexMarvelo please update this PR against latest develop when you have a chance so we can get a green build.
also follow up on #2079 (comment); this PR may not be necessary anymore.

@alxmiron
Copy link
Contributor Author

alxmiron commented Mar 8, 2018

Follow #2216. This PR can be closed

@giladgray giladgray closed this Mar 8, 2018
@giladgray
Copy link
Contributor

sure thing @alxmiron. feel free to close your own PRs in the future 😄

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