breaking(Form|RatingIcon|Search|SearchResult): Pass data to event handlers #951

jeffcarbs · 2016-11-28T16:45:09Z

Breaking Changes!

The following components have breaking changes in their event handler props.

Form

Handler	Previous Signature	Current Signature
onClick	(e, formData)	(e, { ...props, formData })

RatingIcon

Handler	Previous Signature	Current Signature
onClick	(e, index)	(e, { ...props, index })
onKeyUp	(e, index)	(e, { ...props, index })
onMouseEnter	(e, index)	(e, { ...props, index })

Search

NOTE: The handler name changed from onChange to onResultSelect. The change was made because:

onChange doesn't really make sense in this context since nothing is really changing.
given the result is being returned, onChange made even less sense.
it maps closer to the SUI-core event (onSelect). The reason to not use onSelect is because that's a built-in html event type for when a user selects text.

Handler	New Name	Previous Signature	Current Signature
onChange	onResultSelect	(e, result)	(e, result) no change

SearchResult

Handler	Previous Signature	Current Signature
onClick	(e, id)	(e, { ...props, id })

Fixes #623

This updates the common tests for event handling to ensure any handlers that we wrap are called with (event, data) as arguments.

jeffcarbs · 2016-11-28T21:21:20Z

@levithomason - thoughts on this approach?

The alternative would be to have a separate common test like:

common.passesPropsToEventHandler(Component, 'onClick')

The issue with the current approach is that non-built-in events (e.g. onItemClick) aren't tested. That being said, I don't think the suggestion here would work either since there's no way to reliably trigger the event anyway, so we're not really gaining anything.

Also I know we try to avoid big refactors, but assuming we go with this approach (updating the 'handles events' spec) I think it makes sense to just fix all of the handlers at once. These files currently have failing specs:

AccordionTitle
BreadcrumbSection
Checkbox
Dropdown Component
Form
MenuItem
Search Component
SearchResult
Select
Step
TableFooter
TableHeaderCell

codecov-io · 2016-11-28T22:05:54Z

Current coverage is 99.73% (diff: 100%)

No coverage report found for master at 5fb0b69.

Powered by Codecov. Last update 5fb0b69...4411b53

levithomason · 2016-11-29T05:31:40Z

I like the idea of doing these all at once as it makes for less breaking changes and a clear upgrade path for users.

My idea for solving the handled/unhandled callbacks is precisely what you've done. Though, I we can't assert the data param is props because sometimes it has other data like state.value in TextArea and Input. There may be others that have arbitrary data as well. Instead, we should match against an empty object {} for now. Any further data specific logic I think should be the responsibility of individual component test.

jeffcarbs · 2016-11-30T02:32:40Z

calledWithMatch is an alias for calledWith(sinon.match(arg1), sinon.match(arg2), ...) and sinon.match(object) does an incomplete match on objects. So calledWithMatch(event, props) will pass as long as the second arg is an object that includes each key/value in props, even if there are additional key/value.

jeffcarbs · 2016-11-30T03:38:44Z

@levithomason - ready for review 👍

One question I had about Dropdown. Currently we pass { ...this.props, value: proposedValue } as the second argument to onChange but only pass the props to the other handlers. Should we also pass the value to the rest of the event handlers? I was thinking something like:

getHandlerProps = () => {
  return { ...this.props, value: this.state.value }
}

onChange = (e, value /* <- proposed value */) => {
  // ...
  props.onChange(e, { ...this.getHandlerProps(), value })
}

onClick = (e) => {
  // ...
  props.onClick(e, this.getHandlerProps())
}

levithomason · 2016-11-30T18:45:34Z

So calledWithMatch(event, props) will pass as long as the second arg is an object that includes each key/value in props, even if there are additional key/value.

This is correct, however, the test would have failed because the value in the data object is overridden as the event value and props.value != e.target.value. So, we can not be certain that all the props key/value pairs will appear in data.

Should we also pass the value to the rest of the event handlers?

Initially, I don't think this is necessary in this case. When the props change the ACC copies those values to state so the props.value will always equal state.value, except in the case of the onChange where we need to communicate the proposed new value.

That aside, it is a good idea IMO to have a SSOT for the handler props so I still like this idea. It makes testing easier and the component stronger.

layershifter · 2016-11-30T19:30:32Z

For clarity, RatingIcon is internal component and it can't be used outside Rating component. I think it's bad idea to export it.

levithomason · 2016-11-30T18:48:18Z

src/collections/Form/Form.js

@@ -198,7 +198,12 @@ export default class Form extends Component {
    /** Automatically show a loading indicator */
    loading: PropTypes.bool,

-    /** Called with (event, jsonSerializedForm) on submit */
+    /**
+     * Called with (event, jsonSerializedForm) on submit


Probably an oversight here, let's remove the signature from the description.

levithomason · 2016-11-30T18:50:48Z

src/collections/Menu/MenuItem.js

@@ -68,7 +68,13 @@ export default class MenuItem extends Component {
    /** Internal name of the MenuItem. */
    name: PropTypes.string,

-    /** Render as an `a` tag instead of a `div` and called with event on MenuItem click. */
+    /**
+     * Called on click. When passed, the component render as an `a`


Might be missing a word in here: "the component render as an a"

the component will render as an a
the component renders as an a

levithomason · 2016-11-30T18:51:57Z

src/elements/Label/Label.js

@@ -101,10 +101,20 @@ export default class Label extends Component {
      PropTypes.oneOf(_meta.props.pointing),
    ]),

-    /** Adds the link style when present, called with (event, props). */
+    /**
+     * Adds the link style when present, called with (event, props).


Let's remove the signature here as well.

levithomason · 2016-11-30T18:52:05Z

src/elements/Label/Label.js

    onClick: PropTypes.func,

-    /** Adds an "x" icon, called with (event, props) when "x" is clicked. */
+    /**
+     * Adds an "x" icon, called with (event, props) when "x" is clicked.


Another signature we can drop.

levithomason · 2016-11-30T18:53:47Z

src/elements/Step/Step.js

@@ -50,7 +50,13 @@ export default class Step extends Component {
    /** Render as an `a` tag instead of a `div` and adds the href attribute. */
    href: PropTypes.string,

-    /** Render as an `a` tag instead of a `div` and called with event on Step click. */
+    /**
+     * Called on click. When passed, the component render as an `a`


Same comment regarding phrasing here: "the component render as an a"

levithomason · 2016-11-30T18:58:23Z

src/modules/Dropdown/Dropdown.js

    onOpen: PropTypes.func,

-    /** Called with the React Synthetic Event and current value on search input change. */
+    /**
+     * Called with the React Synthetic Event and current value on search input change.


Let's drop the signature info from the description here as well.

levithomason · 2016-11-30T18:58:49Z

src/modules/Embed/Embed.js

@@ -86,7 +86,12 @@ export default class Embed extends Component {
    /** Specifies an icon to use with placeholder content. */
    icon: customPropTypes.itemShorthand,

-    /** Сalled with event on Embed click with (event, props). */
+    /**
+     * Сalled with event on Embed click with (event, props).


Let's drop the signature info from the description here as well.

levithomason · 2016-11-30T18:59:07Z

src/modules/Rating/Rating.js

@@ -71,7 +71,12 @@ export default class Rating extends Component {
      PropTypes.number,
    ]),

-    /** Called with (event, { rating, maxRating }) after user selects a new rating. */
+    /**
+     * Called with (event, { rating, maxRating }) after user selects a new rating.


Let's drop the signature info from the description here as well.

levithomason · 2016-11-30T19:00:05Z

src/modules/Search/Search.js


-    /** Called with the React Synthetic Event and current value on search input change. */
+    /**
+     * Called with the React Synthetic Event and current value on search input change.


Let's drop the signature info from the description here as well.

levithomason · 2016-11-30T19:35:24Z

test/specs/modules/Accordion/AccordionTitle-test.js

 import AccordionTitle from 'src/modules/Accordion/AccordionTitle'

 describe('AccordionTitle', () => {
  common.isConformant(AccordionTitle)
  common.rendersChildren(AccordionTitle)
  common.propKeyOnlyToClassName(AccordionTitle, 'active')
-
-  describe('onClick', () => {


Did we intend to drop these tests or update them?

Intentional given that all the functionality is already being tested by the event-related tests within isConformant

levithomason · 2016-11-30T19:54:15Z

Doc Examples

I'm guessing there are several doc site examples that need to be updated to reflect the new changes as well. Would finding handler prop names in the docs, /on[A-Z]\w+=\{/, and updating where necessary.

RatingIcon

For clarity, RatingIcon is internal component and it can't be used outside Rating component. I think it's bad idea to export it.

@layershifter Good call. We should make this a private component in this case. Simply prefixing its name with _ will make it private. The common tests will then automatically assert that it is not exposed anywhere in the API.

levithomason · 2016-11-30T19:56:46Z

test/specs/commonTests.js

+        let errorMessage = 'was not called with (event)'
+
+        if (_.has(Component.propTypes, listenerName)) {
+          expectedArgs = [eventShape, props]


I see all tests are passing, but how does this work for things like the Input onChange where the data arg's value key != to the props value key? Same for Checkbox's checked prop.

props here isn't the Component's full props object, it's just:

const props = { ...requiredProps, [listenerName]: handlerSpy, 'data-simulate-event-here': true, }

So we're really just testing that arbitrary props get passed through (e.g. that data-simulate-event-here prop). Tests for additional data, like checked of a Checkbox, should be handled in that Component's test file.

jeffcarbs · 2016-12-05T15:09:49Z

I started looking into making RatingIcon a private component and I don't really think we should:

There are currently no other private components, the common tests for them don't actually work out of the box.
While RatingIcon it's not usable outside of Rating, neither is Dropdown.Divider usable outside of Dropdown, nor ItemImage outside of Item.
Our goal is to provide a Component API to give someone full control in rendering all SUI components, so we need to export it to allow full control over a Rating.

levithomason · 2016-12-05T17:15:24Z

That logic makes sense to me, thanks @jcarbo.

levithomason · 2016-12-07T01:26:29Z

Released in [email protected].

jeffcarbs force-pushed the feature/enforce-handler-args branch from 2eba8e1 to 4e1df95 Compare November 28, 2016 16:45

jeffcarbs force-pushed the feature/enforce-handler-args branch from 4e1df95 to c027f6c Compare November 28, 2016 22:05

jeffcarbs force-pushed the feature/enforce-handler-args branch from c027f6c to 5b2eeba Compare November 30, 2016 03:31

jeffcarbs changed the title ~~Feature/enforce handler args~~ breaking(Form|RatingIcon|Search|SearchResult): Pass props to event handlers Nov 30, 2016

jeffcarbs added the ready for review label Nov 30, 2016

levithomason requested changes Nov 30, 2016

View reviewed changes

levithomason reviewed Nov 30, 2016

View reviewed changes

levithomason added needs author feedback and removed ready for review labels Nov 30, 2016

jeffcarbs added 4 commits December 3, 2016 10:45

breaking(multiple): Pass props to callbacks

def69ed

Fix specs, up RatingIcon to be conformant

de975f2

Rename value to formData in onSubmit handler

2c620c4

Update doc comments

9f50d8e

jeffcarbs force-pushed the feature/enforce-handler-args branch from b6c0fca to 9f50d8e Compare December 5, 2016 14:55

jeffcarbs added ready for review and removed needs author feedback labels Dec 5, 2016

Update examples

4411b53

levithomason approved these changes Dec 5, 2016

View reviewed changes

levithomason merged commit fa9175a into master Dec 5, 2016

levithomason deleted the feature/enforce-handler-args branch December 5, 2016 17:16

This was referenced Dec 11, 2016

Form Within Modal - formData submitted returns undefined #1016

Closed

'event.target.value' are 'undefined' of 'onChange(event)' for 'Checkbox', 'Radio', and 'Dropdown' #638

Closed

layershifter mentioned this pull request Jul 3, 2017

breaking(Search): update onResultSelect and onSearchChange signature #1828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

breaking(Form|RatingIcon|Search|SearchResult): Pass data to event handlers #951

breaking(Form|RatingIcon|Search|SearchResult): Pass data to event handlers #951

jeffcarbs commented Nov 28, 2016 •

edited

Loading

jeffcarbs commented Nov 28, 2016 •

edited

Loading

codecov-io commented Nov 28, 2016 •

edited

Loading

levithomason commented Nov 29, 2016

jeffcarbs commented Nov 30, 2016 •

edited

Loading

jeffcarbs commented Nov 30, 2016

levithomason commented Nov 30, 2016

layershifter commented Nov 30, 2016

levithomason Nov 30, 2016

levithomason Nov 30, 2016

levithomason Nov 30, 2016

levithomason Nov 30, 2016

levithomason Nov 30, 2016

levithomason Nov 30, 2016

levithomason Nov 30, 2016

levithomason Nov 30, 2016

levithomason Nov 30, 2016

levithomason Nov 30, 2016

jeffcarbs Dec 5, 2016 •

edited

Loading

levithomason commented Nov 30, 2016

levithomason Nov 30, 2016

jeffcarbs Dec 5, 2016

jeffcarbs commented Dec 5, 2016

levithomason commented Dec 5, 2016

levithomason commented Dec 7, 2016

breaking(Form|RatingIcon|Search|SearchResult): Pass data to event handlers #951

breaking(Form|RatingIcon|Search|SearchResult): Pass data to event handlers #951

Conversation

jeffcarbs commented Nov 28, 2016 • edited Loading

Breaking Changes!

Form

RatingIcon

Search

SearchResult

jeffcarbs commented Nov 28, 2016 • edited Loading

codecov-io commented Nov 28, 2016 • edited Loading

Current coverage is 99.73% (diff: 100%)

levithomason commented Nov 29, 2016

jeffcarbs commented Nov 30, 2016 • edited Loading

jeffcarbs commented Nov 30, 2016

levithomason commented Nov 30, 2016

layershifter commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffcarbs Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason commented Nov 30, 2016

Doc Examples

RatingIcon

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffcarbs commented Dec 5, 2016

levithomason commented Dec 5, 2016

levithomason commented Dec 7, 2016

jeffcarbs commented Nov 28, 2016 •

edited

Loading

jeffcarbs commented Nov 28, 2016 •

edited

Loading

codecov-io commented Nov 28, 2016 •

edited

Loading

jeffcarbs commented Nov 30, 2016 •

edited

Loading

jeffcarbs Dec 5, 2016 •

edited

Loading