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(time_field): Add event in handle onChange, add inputRef #6

Conversation

leonidkuznetsov18
Copy link

Add event for handle onChange. Add inputRef for native DOM element input.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.684% when pulling fcb81d2 on MrChe:feature/time_field into 8a32a41 on antonfisher:master.

src/index.js Outdated
@@ -59,14 +60,14 @@ export default class TimeField extends React.Component {
showSeconds: PropTypes.bool,
input: PropTypes.element,
colon: PropTypes.string,
style: PropTypes.object
style: PropTypes.object,
Copy link
Owner

Choose a reason for hiding this comment

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

Hi @MrChe,
Could you, please, run npm run prettier and add the changes to the PR, to follow preconfigured style. Thank you!

src/index.js Outdated
value,
style,
onChange: onChangeHandler
defaultValue,
Copy link
Owner

Choose a reason for hiding this comment

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

As I see, there is no need to pass defaultValue separately, because all ...props are already passed to the input above (...props, line 177)

src/index.js Outdated
});
}

return (
<input
type="text"
{...props}
colon={colon}
defaultValue={defaultValue}
Copy link
Owner

Choose a reason for hiding this comment

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

The same there, as I see, there is no need to pass defaultValue separately, because all ...props are already passed to the HTML input above ({...props} line 190)

src/index.js Outdated
});
}

return (
<input
type="text"
{...props}
colon={colon}
Copy link
Owner

Choose a reason for hiding this comment

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

HTML input doesn't have colon property, so this may cause a warning

src/index.js Outdated
value,
style,
onChange: onChangeHandler
defaultValue,
ref: inputRef,
Copy link
Owner

Choose a reason for hiding this comment

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

I can imagine the situation where you need to pass inputRef to the cloned element.
I'm trying to figure out can the same problem be solved by replacing this code:

<TimeField input={<MyInput />} inputRef={myRef} />

By this one:

<TimeField input={<MyInput ref={myRef} />} />

?

Copy link
Author

Choose a reason for hiding this comment

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

inputRef need only native DOM input, maybe i don't understand your code

src/index.js Outdated
const onChangeHandler = (event) => this.onInputChange(event, (v) => onChange(v));
const { value } = this.state;
const { onChange, style, showSeconds, input, inputRef, colon, defaultValue, ...props } = this.props; //eslint-disable-line no-unused-vars
const onChangeHandler = (event) => this.onInputChange(event, (v) => onChange(event, v));
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this is good to have!

@antonfisher
Copy link
Owner

Hi @MrChe,
Thanks for the PR, please check out the comments.

Add `event` for handle onChange. Add inputRef for native DOM element input.
fix(index):delete other props
@Nass30
Copy link

Nass30 commented Jul 7, 2020

Hello @MrChe !

I need inputRef too but this PR is really old and cannot be merged, maybe we could just close this one so I can open another one and add it myself ?

Thanks !

@antonfisher
Copy link
Owner

Just published https://www.npmjs.com/package/react-simple-timefield/v/3.2.0 with new inputRef prop.

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