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.

Sign up for GitHub

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

Jump to bottom

[forms] Fix number validation msg #2552

Merged
merged 14 commits into from
May 25, 2021
Merged

[forms] Fix number validation msg #2552

merged 14 commits into from
May 25, 2021

Conversation

andrew-hwahin
Copy link
Contributor

@andrew-hwahin andrew-hwahin commented May 19, 2021 •
edited
Loading

Fix issue #2551

Following is the verified result.

image

image

Sorry for the irrelevant commits, I'm still learning how to fork and contribute. 😅

@andrew-hwahin andrew-hwahin changed the title Fix number validation msg [forms] Fix number validation msg May 19, 2021
@Tobbe Tobbe mentioned this pull request May 19, 2021
Closed
@Tobbe
Copy link
Member

Tobbe commented May 19, 2021

You know what would be great? Some tests for this. It's obviously something we're totally missing right now 😅

Do you think you could have a look at adding tests for all the validation messages?

@Tobbe
Copy link
Member

Tobbe commented May 19, 2021

And hit me up on Discord if you want any help with forking or other git stuff. I can jump on a call and show you.

@andrew-hwahin
Copy link
Contributor Author

I have not written any test before and would need some time to learn how to do so, should we create a separate issue for that?

Previously I performed the modification for this pull request #2487 on the "main" branch of my fork, I think that is why when I open a new branch to fix this issue, all the commits are following along, should I re-fork again?

@Tobbe
Copy link
Member

Tobbe commented May 19, 2021

For running the tests I can recommend giving this a read ;) http://tlundberg.com/blog/2020-06-21/running-specific-tests-redwoodjs/ (Not biased at all! 😛)

@Tobbe
Copy link
Member

Tobbe commented May 19, 2021

Previously I performed the modification for this pull request #2487 on the "main" branch of my fork, I think that is why when I open a new branch to fix this issue, all the commits are following along, should I re-fork again?

You only need one fork. But it's good practice to always create a new branch when you want to work on a new PR. But, no worries, git is awesome in that there is (almost) always a way fix whatever screw-up one might have done 😁 In this case I'd reset your branch for this PR to upstream/main, and then cherry-pick the commit(s) you've already done for this fix, and finally force-push to origin/main. Then you have your branch in a clean state where you can continue pushing extra commits, like for tests for example.

@jtoar jtoar added this to the next-release-candidate milestone May 19, 2021
@jtoar
Copy link
Contributor

jtoar commented May 21, 2021

Closes #2551

@thedavidprice
Copy link
Contributor

Thanks for this @andrew-hwahin! I'm merging now.

I'll let you and @Tobbe discuss a possible follow-up PR for tests. But I agree let's separate into two parts.

@thedavidprice thedavidprice merged commit 7660385 into redwoodjs:main May 25, 2021
@jtoar jtoar linked an issue May 25, 2021 that may be closed by this pull request
[forms] Inverted number validation message #2551
Closed
dac09 added a commit to dac09/redwood that referenced this pull request Jun 2, 2021
@dac09
Merge branch 'main' of github.com:redwoodjs/redwood into fix/jest-fil…
8c34c2c 
…ter-tests

* 'main' of github.com:redwoodjs/redwood:
  downgrade jest-watch-typeahead 0.6.3 (redwoodjs#2699)
  Exclude yarn packages cache. (redwoodjs#2697)
  build(deps): bump ts-morph from 10.1.0 to 11.0.0 (redwoodjs#2656)
  build(deps): bump core-js from 3.12.1 to 3.13.1 (redwoodjs#2680)
  bump react types and eslint packages patch version (redwoodjs#2695)
  build(deps): bump esbuild from 0.12.1 to 0.12.5 (redwoodjs#2654)
  upgrade misc packages with patch (redwoodjs#2694)
  Fix lerna canary publishing; use default --canary versioning with `git describe` (redwoodjs#2693)
  Upgrade axios due to security alert. (redwoodjs#2688)
  add esbuild config to CLI build and dev (redwoodjs#2564)
  set up yarn offline cache (redwoodjs#2669)
  Create file watch for type def generation (redwoodjs#2614)
  Pin package dependencies, remove CRWA template yarn.lock, set up Yarn offline cache (redwoodjs#2637)
  Fix serve tests (redwoodjs#2668)
  Add script to create a functional test project using the latest CRWA template (redwoodjs#2324)
  build(deps): bump @typescript-eslint/eslint-plugin from 4.24.0 to 4.25.0 (redwoodjs#2627)
  Manage history state length exploding if clicking on a link with with the current route location (redwoodjs#2616)
  [forms] Fix number validation msg (redwoodjs#2552)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tobbe Tobbe Tobbe approved these changes

Assignees
No one assigned
Labels
topic/forms
Projects
None yet
Milestone
v0.33.0
Development

Successfully merging this pull request may close these issues.

[forms] Inverted number validation message
5 participants
@andrew-hwahin @Tobbe @jtoar @thedavidprice @AndrewLamYW