Warn about future changes to onChange callback [WIP] #1350
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
interactivellama
force-pushed
the
event-parameter-standardize
branch
from
adeb8d5 to
317ba96
Compare
interactivellama
changed the title
interactivellama
force-pushed
the
event-parameter-standardize
branch
from
f7eb975 to
bf80654
Compare
This was referenced May 18, 2018
interactivellama
force-pushed
the
event-parameter-standardize
branch
2 times, most recently
from
3636db4 to
f880825
Compare
* The audit showed that checkbox and inline edit's `onChange` are wrong (along with deprecated lookup's onChange, onBlur, onFocus, but we aren't updating Lookup). DataTable's `onChange` is also `(selectedArrayOfItems, event)`. I'm going think about that one a little more. * SLDS has moved their form components out from under the form folder, so a breaking change in event parameters was a good time to move the `checkbox` component out of the forms folder. That way, consumers can upgrade at their leisure--unless they are using the CommonJS named imports. * Right now, CommonJS and `components/forms/checkbox` users (that is everyone) will get the warning, but nothing will break. * Went ahead and converted checkbox to a ES6 class and removed a lodash function. * There was a comment in all the site-stories that mentioned `forms-checkbox` and that has been removed. * The first commit "Align event prop parameters" just adds from empty objects to a few components as the second parameter `data` object to existing event callbacks, just to guard from being `undefined` if accessed. This should be standardized. * If oldEventParameterOrder is true, a warning will fire. This will tell folks to change their path—if they are consuming the source code. The CommonJS named imports will just have to break at the next breaking change due to the parameter change, since you can’t change the path.
interactivellama
force-pushed
the
event-parameter-standardize
branch
from
86248c4 to
e8c984e
Compare
…to event-parameter-standardize2
components/checkbox/check-props.js
Outdated
| propAsString: 'onChange', | ||
| propAsValue: props.onChange, | ||
| }, | ||
| '`components/forms/checkbox` is deprecated. `components/checkbox` should be used. When this path update is made `onChange` event parameters will change from `onChange(value, event, { value } to `onChange(event, { value }). Please update your event parameters when you change paths.` If you are using the CommonJS named import, `Checkbox` events will break at v1.0 and this warning will be present until then. Please review https://github.com/salesforce/design-system-react/releases when you upgrade.' |
There was a problem hiding this comment.
Minor: missing ) in onChange(value, event, { value } to ...
components/checkbox/check-props.js
Outdated
| '`components/forms/checkbox` is deprecated. `components/checkbox` should be used. When this path update is made `onChange` event parameters will change from `onChange(value, event, { value } to `onChange(event, { value }). Please update your event parameters when you change paths.` If you are using the CommonJS named import, `Checkbox` events will break at v1.0 and this warning will be present until then. Please review https://github.com/salesforce/design-system-react/releases when you upgrade.' | ||
| ); | ||
|
|
||
| if (props.variant === 'toggle' && props.indeterminate === true) { |
There was a problem hiding this comment.
Minor : === true not necessary?
…to event-parameter-standardize2 # Conflicts: # package.json
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #273
Additional description
/formsthat are not deprecated, aliases have been added to make backwards compatible. The current code is NOT a breaking change, but it will update the documentation to promote use of/components/checkboxinstead.onChangeparameters are wrong (along with deprecated lookup's onChange, onBlur, onFocus, but we aren't updating Lookup).onChangeis also(selectedArrayOfItems, event). I'm going think about that one a little more. I'm thinkingonRowChangeoronRowSelectwould be the best backwards compatible option right now.checkboxcomponent out of the forms folder. That way, consumers can upgrade at their leisure--unless they are using the CommonJS named imports.components/forms/checkboxusers (that is everyone) will get the warning, but nothing will break.forms-checkboxand that has been removed.dataobject to existing event callbacks, just to guard from beingundefinedif accessed. This should be standardized.Deprecation warning for InlineEdit
Checkbox warning:
Pull Request Review checklist (do not remove)
npm run lint:fixhas been run and linting passes.components/component-docs.jsonCI tests pass (npm test).last-slds-markup-reviewinpackage.jsonand push.last-accessibility-review, topackage.jsonand push.npm run local-updatewithin locally cloned site repo to confirm the site will function correctly at the next release.