Polaris-react: Not receiving an event for onChange

[bakura10]

Issue summary

The documentation of text field (https://polaris.shopify.com/components/forms/text-field#navigation) indicates that the onChange event is supposed to received the new value and an event with all the details about the input.

However, what I'm receiving here is the value as first parameter, and the input ID as second parameter. There is no way to get the event.

I'd like rather to keep Polaris generating a unique ID for me and instead use the "name" (get from the event) to update our fields. As of today I've created a small function that prevents me to define callbacks for all fields:

updateField = (value, id) => {
    this.setState({
      [id]: value
    });
  }

Expected behavior

Instead I'd like to be able to do that:

updateField = (value, event) => {
    this.setState({
      [event.target.name]: value
    });
  }

Is this a bug of the documentation or a misunderstanding from me ? :)

EDIT: apparently #83 said that this is the expected result, so I think the documentation is misleading ;).

[lemonmade]

Hey @bakura10, you're right that the docs are wrong, we'll get an update for this out soon. In general, avoiding the unique ID generation is good because it will be different between server and client (which is a performance issue when relying on server rendering). In general, you should be able to use the same name as id, and if not, you can typically have a pretty clear mapping between those two values.

[theodoregold]

I have a similar problem. Just checked the source and this is a big no, no. We have to be able to get the event data.

@autobind
private handleChange(event: React.ChangeEvent<HTMLInputElement>) {
    const {onChange} = this.props;
    if (onChange == null) { return; }
    onChange(event.currentTarget.value, this.state.id);
}

Why not also pass the event?

onChange(event.currentTarget.value, this.state.id, event);

[celsowhite]

Because we can't access the event, I'm getting around this by doing the following.

Creating a method to handle input changes. In this method I can invoke other functions (that is the main reason I'm not inlining the setState into the input.

handleInputChange(name, val) {
   this.setState({
      [name]: val
   });
}

Adding an onChange event to my inputs that passes the input value and my own custom name parameter.

<TextField label="Image src" value={this.state.src} name="src" onChange={val => this.handleInputChange('src', val)} />

[sniepoort]

I find the lack of passing the event param pretty frustrating. Is there any logical reason why it's not included?

[lemonmade]

We haven't needed it in our internal use of the component, and we want to make it easy to do the common thing (using the new value, and not reaching directly into the input) — we've either created a callback bound to the specific input, or we've used the second argument to get the ID which uniquely identifies the input for us. We are aware of an issue where using the ID doesn't particularly help for radio buttons, we hope to have that fixed in a future version.

[sniepoort]

I realize that for your internal use, it is not a necessary feature. But since this package is also meant to be used by other developers, I still don't see an argument for not exposing the event. It could easily be passed as a third parameter, without any breaking changes. And seeing as several people have mentioned this as being useful (and in our case, essential), it would be great if you would consider implementing it.

[lemonmade]

Can you describe why it is essential for your use case?

[dylan]

In my case, I want to use event.persist() like you can anywhere else in React. (see here)

Edit: This ended up not being needed after all.

[lemonmade]

What do you want from the event that you need to persist? The persisted object isn’t really useful, there must be properties bar you are looking to grab that we don’t currently give you.

[dylan]

I was wrong in my case. Thank you for your response!

[ry5n]

Right now we’re happy with the current API, but if you’re still running into problems with this, please let us know.

[jejacks0n]

Ok, so I think I'm running into a similar problem, and the core of it seems to be the TypeScript @autobind decorator. I'm not familiar with TypeScript so I'm hoping @ry5n can reopen the issue and we can discuss it further.

My use case isn't about events. I'm only trying to provide custom behavior for ResourceList.prototype.renderItem (as found here). My objective is to make a drag/drop sortable resource list, and so I need access to the li that's being rendered in that method, but there's literally no possibility of me ever being able to because of the way @autobind works.

The primary problem is not understanding the difference between what @autobind is doing, and the behavior behind .bind(this)

This took me a good hour, hour and a half to debug, identify and confirm. I'm an experienced dev, and found this crazy frustrating. I struggle to think that this limitation is intentional and that the API is that no autobound method can be overridden in subclasses.

Unrelated, but similarly, you may want to declare some of these methods as protected and not private, based on how TypeScript seems to handle both for inheritance.

[jejacks0n]

By the way, it's been super nice using Polaris thus far. I just think this is a limitation that might not be fully understood.

[lemonmade]

@jejacks0n I think your issue is different enough to warrant a separate issue. In short, though: it is an intentional design decision, our use of private variables (and whatever decorators we choose to use on methods) is not meant to be anything a consumer would ever need to know, and subclassing React components is not a common pattern that we intend to support (at any time, we might switch to a function component, and it would not be a breaking change). Use cases that would require overriding methods are probably either indicative of a request for additional props, or an indication that the component in question probably isn’t right for the use case.

[jejacks0n]

That's reasonable and I appreciate the response @lemonmade. Follow up question before I create an issue. Does drag sorting of items within a ResourceList seem like I'm using the wrong component, or does it seem like we should try to expose the ability to do this via a PR or otherwise? It drives how I present the new issue and how much time I'm willing to invest in it.

Also, are Polaris components never inherited inside shopify's own usage of it? I feel like as a developer who understands inheritance, composition etc. that if the code is there and the pattern is there I can and should feel comfortable using it -- and in doing so, understand the risks and situation I choose to put myself and my project in. The usage of @autobind literally makes it a black box that I can't use and I'd still like to discuss the usage of @autobind vs. .bind(this) somewhere.

[lemonmade]

I would defer to @ry5n on whether dragging and dropping is a sensible thing to have within a ResourceList (I am going to guess that it has not been considered a core part of that component, given that we don't have UIs in our main product that does such a thing with lists of resources; the product picker on the collections page is probably the closest thing but it uses a different component internally).

To answer your second question, no, in all of our codebase that has been migrated to React, we never inherit from any React component, from Polaris or elsewhere. In cases where we need behaviour not supported by inheritance, we've forked the component into a project, made the changes, and then either stuck with the fork (if it is was very unique) or committed those changes upstream into Polaris itself.

[ry5n]

ResourceList currently works well for listings that support these behaviors (number 1 is assumed for all resource lists, others are optional):

1. Browse and navigate by clicking an individual item
2. Filtering and sorting the list to find something specific
3. Multiple selection and bulk actions on the items

I considered drag-and-drop as a potential use case, but the component is complex enough that I don’t think I would use it for that, today. There’s just not enough in common between those behaviors and drag-and-drop.

I’m happy to chat about the specific interaction you’re working on, in a separate issue. Getting concrete about the UX problem often helps illuminate what components to use, whether to build anything new, and how to combine things.

[thomasraydeniscool]

Sorry to revive a dead issue but because the event isn't passed with onChange, I can't use a form library such as Formik with Polaris which is frustrating...

[eballeste]

@thomasraydeniscool i'm using formik with polaris, works fine...

const CustomText = ({ field, form: { errors }, ...props }) => {
  if (errors[field.name]) {
    props.error = errors[field.name];
  }
  return (
    <TextField {...field} {...props} />
  );
};

<Formik
  initialValues={{ name: ''}}
  validationSchema={RuleGroupSchema}
>
  {({ setFieldValue }) => (
    <Field
      name="name"
      label="Name"
      component={CustomText}
      onChange={value => { setFieldValue('name', value) }}
    />
    <Button submit>Submit</Button>
  )}
</Formik>