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): prop to return the active result #1825

Merged
merged 3 commits into from
Aug 10, 2017

Conversation

treyhuffine
Copy link
Contributor

Return the active value in <Search> when navigating using the arrow keys

* @param {SyntheticEvent} event - React's original SyntheticEvent.
* @param {object} data - All props.
*/
onActiveSelectionChange: PropTypes.func,
Copy link
Member

Choose a reason for hiding this comment

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

I think that onActiveSelectionChange can be renamed to onSelectionChange.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

@treyhuffine thanks for PR.

I left some feedback, it will awesome if we will also add new test for the prososed callback.

debug('handleActiveSelectionChange()')
const { onActiveSelectionChange } = this.props
const result = this.getSelectedResult()
if (onActiveSelectionChange) onActiveSelectionChange(result)
Copy link
Member

Choose a reason for hiding this comment

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

We can exclude there if-condition if we will use lodash's invoke. Also, our callbacks should return props as the second argument.

_.invoke(this.props, `onActiveSelectionChange`, e, { ...this.props, result})

@treyhuffine
Copy link
Contributor Author

@layershifter thanks, I'll fix and resubmit.

@layershifter
Copy link
Member

@treyhuffine Thanks 👍 You can simply push your commits into the current branch instead of the new PR

@codecov-io
Copy link

codecov-io commented Jul 11, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@2b906ed). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1825   +/-   ##
=========================================
  Coverage          ?   99.75%           
=========================================
  Files             ?      145           
  Lines             ?     2478           
  Branches          ?        0           
=========================================
  Hits              ?     2472           
  Misses            ?        6           
  Partials          ?        0
Impacted Files Coverage Δ
src/modules/Search/Search.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 2b906ed...f6bac16. Read the comment docs.

@@ -301,11 +316,11 @@ export default class Search extends Component {
switch (keyboardKey.getCode(e)) {
case keyboardKey.ArrowDown:
e.preventDefault()
this.moveSelectionBy(1)
this.moveSelectionBy(e, 1)
Copy link
Member

Choose a reason for hiding this comment

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

We will need e in handleSelectionChange.

@@ -105,7 +105,7 @@ export interface SearchProps {
* @param {SyntheticEvent} event - React's original SyntheticEvent.
* @param {object} data - All props.
*/
onResultSelect?: (event: React.MouseEvent<HTMLDivElement>, data: SearchProps) => void;
onResultSelect?: (event: React.MouseEvent<HTMLDivElement>, data: SearchResultData) => void;
Copy link
Member

Choose a reason for hiding this comment

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

data there also contains result.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

@levithomason we're for review there.

@levithomason levithomason merged commit 8e2ae3d into Semantic-Org:master Aug 10, 2017
@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.

4 participants