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

UI - masked input onchange #6586

Merged
merged 3 commits into from
Apr 15, 2019
Merged

UI - masked input onchange #6586

merged 3 commits into from
Apr 15, 2019

Conversation

meirish
Copy link
Contributor

@meirish meirish commented Apr 15, 2019

This fixes an issue where the masked-input component wasn't firing onchange events when the value changed when it was used in conjunction with the form-field component.

To test:
Enable the OIDC auth method in the UI
Fill out the client secret field
Save the form
Observe that the the secret field gets sent to the server in the browser dev tools

Thanks for catching this @yhyakuna !

@meirish meirish added this to the 1.1.2 milestone Apr 15, 2019
@meirish meirish requested review from a team April 15, 2019 16:00

**Example**

```js
<MaskedInput
@value={{attr.options.defaultValue}}
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the example code here got cut off! unfortunately this is one of the bugs with jsdoc2md -- it doesn't like it when components are on multiple lines. unfortunately you'll have to copy/paste the example from the component source file. womp. :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no :( I'll fit that, that's rather unfortunate though

* />
* ```
*
* @param [value] {String} - The value to display in the input.
* @param [placeholder=value] {String} - The placeholder to display before the user has entered any input.
* @param [allowCopy=null] {bool} - Whether or not the input should render with a copy button.
* @param [displayOnly=false] {bool} - Whether or not to display the value as a display only `pre` element or as an input.
* @param [onChange=false] {Function|action} - A function to call when the value of the input changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

yay, storybook updates in action! thank you for remembering to do this!

Copy link
Contributor

Choose a reason for hiding this comment

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

oh also, should the default value of onChange be null instead of false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, copy and paste error - the default is an empty fn, I'm not sure how to express that here... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i noticed that. i think you could try putting an empty fn in here and if that doesn't work, i think you could just omit the default value and list it like so:
* @param [onChange] {Function|action} - A function to call when the value of the input changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with Function.prototype since all code gets escaped and garbled. Function.prototype is also an empty fn, so I think that works for now. Would be nice to do () => {} or similar instead though.

@@ -138,6 +138,7 @@
@value={{or (get model valuePath) attr.options.defaultValue}}
@placeholder=""
@allowCopy="true"
@onChange={{action (action "setAndBroadcast" valuePath)}}
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the difference between the inner action (inside the parentheses) and outer action (inside the curly braces) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the outer one calls a curried version returned by the inner one, onChange just passes the value, but the inner one returns a curried setAndBroadcast with path already set.

@andaley
Copy link
Contributor

andaley commented Apr 15, 2019

this works in the ui, yay! other than my questions and the storybook fixes, looks good 2 go!

@andaley andaley self-requested a review April 15, 2019 22:51
@meirish meirish merged commit 8f44742 into master Apr 15, 2019
@meirish meirish deleted the ui-masked-input-onchange branch April 15, 2019 22:54
briankassouf pushed a commit that referenced this pull request Jun 4, 2019
* update masked-input to work with form-field component

* add test for masked input onChange functionality

* fix doc changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants