-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
fix(editor): Int/Float Editors multiple small enhancements #54
Conversation
- should be type number not text - onCellChange should not trigger on 0 when unchanged - changed styling so that input are not invisible, however also add SASS variables in case user still want previous styling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice PR. I actually had changing the input to type number
in a list of PRs i was eventually going to do and submit
@@ -34,7 +36,7 @@ export class IntegerEditor implements Editor { | |||
} | |||
|
|||
loadValue(item: any) { | |||
this.defaultValue = item[this.args.column.field]; | |||
this.defaultValue = parseInt(item[this.args.column.field], 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just out of curiosity, what benefit does parseInt
provide in loadValue
and isValueChanged
? I see it converts the value from the string value to compare. Would comparing the string value provide the same result without the overhead of parsing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this parseInt
, the isValueChanged
function was returning True
when clicking on Inline Editor without changing the value, because the string was not equal to a number and so this was also triggering a onCellChanged
which was unwanted. It was mostly obvious with a "0" in the input and I didn't want that behavior. You can see this by modifying the Example 3 and connecting the onCellChanged
I'm not sure why but I only had to do this code change in the Integer Editor, not the Float Editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmzagorski
are you ok with the change? I'd like to get this PR merged by the end of week. I'm planning on a new release during the weekend. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes looks good from what i can see
Integer & Float Editors multiple small enhancements
onCellChange
should not trigger on0
when input is really unchanged@jmzagorski
Just adding you as reviewer to make you inform of the PR since I know you use some Inline Editors