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

Always omit "callbackId" prop #714

Merged
merged 7 commits into from
Feb 27, 2017
Merged

Always omit "callbackId" prop #714

merged 7 commits into from
Feb 27, 2017

Conversation

jondlm
Copy link
Contributor

@jondlm jondlm commented Feb 14, 2017

PR Checklist

  • Manually tested across supported browsers
  • Unit tests written (common at minimum)
  • PR has one of the semver- labels
  • Two core team engineer approvals
  • One core team UX approval

export function omitProps(props, type, keys = []) {
return _.omit(props, _.keys(type.propTypes).concat(keys).concat(['initialState']));
return _.omit(props, _.keys(type.propTypes).concat(keys).concat(['initialState', 'callbackId']));
Copy link
Contributor

Choose a reason for hiding this comment

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

We talked about this a little bit, but if our goal is for this prop to cover all closure cases, I think it would be nice if the name were more general rather than tied to id.

Ideas: callbackProp, callbackProps, callbackValue.

Copy link
Contributor Author

@jondlm jondlm Feb 21, 2017

Choose a reason for hiding this comment

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

After thinking a little bit about this, it seems like the 95% case is going to be an id and we should name it accordingly. It also encourages people to keep use something simple that's uniquely identifiable.

Yeah it technically can be object, but I don't think that's something we need to advertise.

@jondlm
Copy link
Contributor Author

jondlm commented Feb 21, 2017

Found a bug w/ RadioGroup (thanks tests!) which uses callbackId internally and excepts it to be passed through a couple layers of lucid components.

@mute
Copy link
Contributor

mute commented Feb 21, 2017

It's probably bad that my initial instinct was, "The tests must be wrong"...

@mute
Copy link
Contributor

mute commented Feb 21, 2017

@codecov-io
Copy link

codecov-io commented Feb 24, 2017

Codecov Report

Merging #714 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #714      +/-   ##
==========================================
+ Coverage   91.46%   91.46%   +<.01%     
==========================================
  Files         157      157              
  Lines        2682     2684       +2     
  Branches      743      744       +1     
==========================================
+ Hits         2453     2455       +2     
  Misses        219      219              
  Partials       10       10
Impacted Files Coverage Δ
src/util/component-types.js 95.55% <100%> (+0.2%)

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 9f77744...6c978c3. Read the comment docs.

@jondlm jondlm merged commit fa67f44 into master Feb 27, 2017
@jondlm jondlm deleted the add-special-omitted-prop branch February 27, 2017 18:53
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.

5 participants