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

Add float field type for keystone-next #4907

Merged
merged 8 commits into from
Feb 23, 2021

Conversation

MurzNN
Copy link
Contributor

@MurzNN MurzNN commented Feb 21, 2021

I created a float field for Keystone-next, based on code from integer field type with some modifications. Please review it and comment if something done wrong.
If all is done right, I will try to implement double field type via same way.

How to test:

  1. In examples-next/basic/schema.ts add the float field type, for example, to "Posts" list:
   Post: list({
     fields: {
       ...
+      float: float(),
        ...
     }
   }),
  1. Try to create, update, remove items in "Posts" list - all should work well.

Tested with mongoose and knex adapters.

Based on integer field type with some modifications
@changeset-bot
Copy link

changeset-bot bot commented Feb 21, 2021

🦋 Changeset detected

Latest commit: b02dd7b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@keystone-next/fields Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Feb 21, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/keystonejs/keystone-next-docs/2a2nZEJiiHNPS33gQDgCUQi9VcVA
✅ Preview: https://keystone-next-git-fork-murznn-keystone-next-float-field-3a26ca.vercel.app

@vercel vercel bot temporarily deployed to Preview February 21, 2021 15:38 Inactive
@vercel vercel bot temporarily deployed to Preview February 21, 2021 15:41 Inactive
@vercel vercel bot temporarily deployed to Preview February 21, 2021 17:11 Inactive
@timleslie
Copy link
Contributor

This all looks good to me, thanks for the PR @MurzNN 🙏

@mitchellhamilton Could you please review the Admin UI component and if you're happy with it, give this a ✅ Thanks!

@vercel vercel bot temporarily deployed to Preview February 22, 2021 03:04 Inactive
@timleslie timleslie requested a review from emmatown February 22, 2021 21:04
packages-next/fields/src/types/float/views/index.tsx Outdated Show resolved Hide resolved
packages-next/fields/src/types/float/views/index.tsx Outdated Show resolved Hide resolved
graphqlSelection: config.path,
defaultValue: '',
deserialize: data => (parseFloat(data[config.path]) || '') + '',
serialize: value => ({ [config.path]: parseFloat(value.replace(',', '.')) || null }),
Copy link
Member

Choose a reason for hiding this comment

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

This logic should be identical to the logic in integer except it should call parseFloat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this logic I wanted to add support for , as decimal separator together with ., because it is popular in many countries
https://en.wikipedia.org/wiki/Decimal_separator#Countries_using_decimal_comma
So pasting something like 3,14159 should be converted to 3.14159 instead of 314159 or 3.
But this is debatable change, so now let's keep the previous logic to merge this, I've reverted this line to integer logic.

Co-authored-by: Mitchell Hamilton <[email protected]>
@vercel vercel bot temporarily deployed to Preview February 23, 2021 08:57 Inactive
@vercel vercel bot temporarily deployed to Preview February 23, 2021 23:12 Inactive
@emmatown emmatown merged commit 53b8b65 into keystonejs:master Feb 23, 2021
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.

3 participants