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(Search): add handling of html input props #1442

Merged
merged 5 commits into from
Mar 14, 2017
Merged

Conversation

layershifter
Copy link
Member

Fixes #1437.

@codecov-io
Copy link

codecov-io commented Mar 11, 2017

Codecov Report

Merging #1442 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1442      +/-   ##
==========================================
- Coverage   99.74%   99.74%   -0.01%     
==========================================
  Files         141      141              
  Lines        2364     2363       -1     
==========================================
- Hits         2358     2357       -1     
  Misses          6        6
Impacted Files Coverage Δ
src/modules/Search/Search.js 100% <100%> (ø)
src/elements/Input/Input.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d72812...da55e17. Read the comment docs.

@levithomason
Copy link
Member

I think we need to implement the mechanism that the Input itself implements, so that all HTML <input> props are forwarded to the input.

We can pull this list of HTML input props out of Input.js and into /lib inside a method like getHTMLInputProps(props) so that it can be used in both components:

import { getHTMLInputProps } from '../lib'

const htmlInputProps = getHTMLInputProps(props)

@layershifter
Copy link
Member Author

Makes sense to me. Will implement it.

const value = _.get(e, 'target.value')

const { onChange } = this.props
if (onChange) onChange(e, { ...this.props, value })
Copy link
Member Author

Choose a reason for hiding this comment

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

Cruft. handleChange will be called only if onChange defined.

@layershifter layershifter changed the title feat(Search): add name prop feat(Search): add handling of html input props Mar 12, 2017

export const htmlInputPropNames = [...htmlInputProps, ...htmlInputEvents]

export const omitHTMLInputProps = (props, htmlProps = htmlInputPropNames) => _.omit(props, htmlProps)
Copy link
Member

Choose a reason for hiding this comment

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

Lodash has perf issues with _.omit and it is also planned to be removed in v5, #864 . How about we move these two pick/omit methods into a single reduce that returns a pair of objects? Something like:

const [htmlInputProps, rest]  = partitionHTMLInputProps(props)

Given:

const htmlInputProps = ['selected', ...]

const getHTMLInputProps = props => htmlInputProps.reduce((acc, next) => {
  acc[+!(next in props)] = props[next]
  return acc
}, [])

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion, will make it 👍

@levithomason levithomason merged commit f30aee4 into master Mar 14, 2017
@levithomason levithomason deleted the feat/search-name branch March 14, 2017 20:09
@levithomason
Copy link
Member

Released in [email protected].

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

Successfully merging this pull request may close these issues.

3 participants