Skip to content

Allow setting attributes on editable inputs#1464

Merged
AllenFang merged 3 commits intoAllenFang:v4.0.0-devfrom
jspaine:v4.0.0-dev
Jul 16, 2017
Merged

Allow setting attributes on editable inputs#1464
AllenFang merged 3 commits intoAllenFang:v4.0.0-devfrom
jspaine:v4.0.0-dev

Conversation

@jspaine
Copy link

@jspaine jspaine commented Jul 9, 2017

I think it would be nice if there was an easy way to set attributes for editable inputs without having to create a customEditor. This allows stuff like:

editable={{type: 'number', attrs: {min: 1, max: 4, step: 1}}}

I'd only want it for simple attributes, but I don't like that it just silently ignores anything the user passes for ref, onKeyDown and onBlur and I'm not sure the best way to deal with it. Is it worth the extra code to support these, should ref and on* throw an error...?

let { className } = this.state;
// put placeholder if exist
editable.placeholder && (attr.placeholder = editable.placeholder);
editable.attrs && (attr = {...editable.attrs, ...attr});
Copy link
Owner

Choose a reason for hiding this comment

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

I think following will be simple, keep const and dont assign attr again:

const attr = {
      ref: 'inputRef',
      onKeyDown: this.handleKeyPress,
      onBlur: this.handleBlur,
      ...editable.attrs
};

Please check 👍 thanks

Copy link
Author

Choose a reason for hiding this comment

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

Yeah that's easier 😁 I spread the user supplied attrs first though so the ref and handlers can't be overwritten.

Copy link
Owner

@AllenFang AllenFang left a comment

Choose a reason for hiding this comment

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

LGTM

@jspaine
Copy link
Author

jspaine commented Jul 13, 2017

I added the code to allow refs, keydown and blur handlers. Also a warning 'Setting editable.placeholder is deprecated. Use editable.attrs to set input attributes' so you can eventually get rid of separately setting the placeholder attr.
Does it sound ok? Shall I push it here or do you want to leave it like this?

@AllenFang
Copy link
Owner

@jspaine, it sounds good, feel free to patch PR 👍

@AllenFang AllenFang merged commit be82349 into AllenFang:v4.0.0-dev Jul 16, 2017
@AllenFang
Copy link
Owner

@jspaine, this was fixed and released on v4.0.0-beta.8, thanks your contributions 👍

@jspaine jspaine deleted the v4.0.0-dev branch July 17, 2017 00:07
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.

2 participants