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

New: AlertInput onFocus prop addition #636

Merged
merged 1 commit into from
Oct 18, 2017
Merged

Conversation

adityasabnis
Copy link
Member

@adityasabnis adityasabnis commented Oct 18, 2017

Add onFocus propType function to AlertInput

Resolves: #637

capture

@codecov-io
Copy link

codecov-io commented Oct 18, 2017

Codecov Report

Merging #636 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #636   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          49     49           
  Lines         691    693    +2     
  Branches      144    145    +1     
=====================================
+ Hits          691    693    +2
Impacted Files Coverage Δ
src/components/adslot-ui/AlertInput/index.jsx 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 1fd0ae2...7580eb1. Read the comment docs.

@adityasabnis adityasabnis force-pushed the alert-input-onfocus-prop branch from 5b01d5c to 7580eb1 Compare October 18, 2017 02:10
Copy link
Contributor

@lteacher lteacher left a comment

Choose a reason for hiding this comment

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

lgtm ⭐ 🌟 💥

@@ -46,6 +46,10 @@ export default class AlertInput extends Component {
isFocused: true,
isPopoverVisible: Boolean(this.props.alertMessage),
});

if (this.props.onFocus) {
this.props.onFocus(event);
Copy link
Contributor

Choose a reason for hiding this comment

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

probably just default to _.noop so you don't have to make this check

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree, I think this check is valid and preferable to a noop for consistency with other types and for providing explicitness with the optional parameters.

@adityasabnis
Copy link
Member Author

Discussion outcome for moving to noop: Stick to existing.

@mdotwills
Copy link
Contributor

Merged.. ⏰

@mdotwills mdotwills merged commit a7a2977 into master Oct 18, 2017
@mdotwills mdotwills deleted the alert-input-onfocus-prop branch October 18, 2017 03:30
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.

7 participants