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

number_field for Field::Number #1063

Merged
merged 1 commit into from
Oct 5, 2020
Merged

number_field for Field::Number #1063

merged 1 commit into from
Oct 5, 2020

Conversation

codergeek121
Copy link
Contributor

@codergeek121 codergeek121 commented Jan 8, 2018

I changed the partial for the Field::Number to use the number_field view helper instead of the text_field helper. The tests failed for my cloned master branch, but my first commit fixes it.
Is a new test for the input_field change necessary, and if so what would be a good test to write?

This PR is for Issue #1021.

Happy to help.

@codergeek121 codergeek121 changed the title number_field for Field::Number #1021 number_field for Field::Number Jan 8, 2018
@nickcharlton
Copy link
Member

nickcharlton commented Mar 2, 2018

Hi @codergeek121!

I'm not sure a test would be 100% valuable in this scenario. Is there some validation/presentation that number_field does that we can use to our advantage for one?

Could you also rebase against master? I wonder if that'll now help with that first commit.

@jdrosales17
Copy link

Hi @nickcharlton!

I was just working with the gem and wondering why Field::Number was using text_field, which led me to this PR. Do you know when it will be merge ?

@pablobm
Copy link
Collaborator

pablobm commented Dec 21, 2019

@jdrosales17 - If you rebase this (skipping the first commit, now unnecessary) and create a new PR, I'll be happy to review it.

@nickcharlton nickcharlton added feature new functionality that’s not yet implemented fields new fields, displaying and editing data labels Jan 2, 2020
@nickcharlton
Copy link
Member

nickcharlton commented Oct 5, 2020

I think we're good here. Maybe there will be a regression and we can pick that up but the combination of using the right field type and "any" is good for now. Thanks for your contribution!

@nickcharlton nickcharlton merged commit 74cb093 into thoughtbot:master Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature new functionality that’s not yet implemented fields new fields, displaying and editing data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants