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

[WIP] Add number field #1068

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@Blistok
Copy link
Contributor

commented Mar 23, 2019

WIP - Please don't merge @aheinze.

This PR adds explicit number field. I'm aware of the option to use text field for this purpose (by adding "type": "number" in options (related discussions: link 1, link 2).
The issue I'm facing with current solution is that, even though the UI recognizes the type number field and renders appropriate input tag, the actual value gets converted in 2 places from number to string.
First (implicit conversion) happens when doing JSON serialization here, and the second (explicit) one just before saving here.
Also, it seems that @aheinze already had this use case in mind judging by this commit and this comment.

So it seems that we have 2 options, either to fix this return type or to switch to having explicit number field type (this PR). Pro of the first approach is that it would be less code (probably could be done with one line surgically in particular place). Pros of the second approach are, even though we end up with more code, we have benefit of transparency (the whole discussion from link 1 and link 2 above wouldn't be happening in the future) and by not casting values (especially in multiple places) we would avoid source of subtle bugs (it took me a while to figure out why my sort function on one of the "number" fields didn't give expected results on the frontend before I realize that it wasn't sorting by number values but by ASCII string values!).
Finally, should we decide to go this route we'll have to consider plugins support. For example, CockpitQL would have to update it's types list before it could work with the new number field.

Related issue: #1062

@Blistok Blistok changed the title [WIP]Add number field [WIP] Add number field Mar 23, 2019

@aheinze

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

is this still needed after this commit: 781c44e ?

@Blistok

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

So, that one seems to solve the implicit conversion (the first one I mentioned in the original description), but then when it gets saved it gets explicitly converted from number to string here.

Simple test case is to put one console.log for this.entry just before this line and one for entry inside .then() when promise resolves after saving the entry; and you should see in the first log it will return number, and in the second it will be converted to string.

@Blistok

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@aheinze I'm gonna close this PR. I ended up doing separate mini addon for the number field here:
https://github.com/Blistok/CockpitCMS-NumberField

@Blistok Blistok closed this May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.