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

Extend Numbers field, various improvements #8

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

timkelty
Copy link
Collaborator

Fixes #1, #3

Summary of changes:

  • Field now directly extends craft\fields\Number, which removes a lot of duplicate code, allows defaultValue to be null, and gives us prefix/suffix.
  • The matching field setting defaults now match Number:
    • defaultValue = null
    • size = null
    • decimals = 0
  • Input Template
    • Field now in a flex wrapper (like Number field) to lay things out horizontally
    • unit is displayed using forms.textField's unit option

Tim Kelty added 10 commits January 22, 2020 13:18
Avoid duplicating defaults by simply setting default props. No need to do this AND use a default validator.
This was we don't have to replicate the min/max/defaultValue/decimals/size handling, and we can leverage the parent's normaliztion.
@khalwat
Copy link
Contributor

khalwat commented Feb 14, 2020

So... do you need me to review this or... do you just wanna merge it yourself?

@nitech
Copy link

nitech commented Dec 8, 2020

Heeey, any reason this is not merged?

@khalwat
Copy link
Contributor

khalwat commented Dec 8, 2020

Waiting on @timkelty

@timkelty
Copy link
Collaborator Author

Same here.

@timkelty
Copy link
Collaborator Author

timkelty commented Dec 11, 2020

@nitech FWIW, I am using this on a production site…

I didn't have time to properly review it, which is why it remains unmerged.
On my list, though.

@nitech
Copy link

nitech commented Dec 11, 2020

I understand. From what I see in the comments, @khalwat says he approves the code..?

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