-
Notifications
You must be signed in to change notification settings - Fork 13
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 form error messages #675
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just remove the unnecessary lines I mentioned below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the way you handle invalid emails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure you address the other comments and then rebase your code.
The Travis bot keeps saying "Some checks haven't completed yet" and "Expected - Waiting for status to be reported". Can you help me out with that, thanks! I tried git push force but nothing changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests succeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the email check, probably add a confirm password option
719b452
to
2c20523
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
branch needs to be updated!
Can you elaborate? |
it says that your branch is 7 commits behind dev, meaning your branch is out of date. Checkout wiki for the steps to rebase, if its your first time me or isita can help you! |
oh ok, I will look into the wiki first and try to do it. Thanks |
0a52d23
to
f170478
Compare
Branch rebased |
c0254c4
to
871c99d
Compare
f170478
to
bc87f25
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some errors.
Co-authored-by: charlotte <61573792+charlotte-zhuang@users.noreply.github.com>
Co-authored-by: charlotte <61573792+charlotte-zhuang@users.noreply.github.com>
Co-authored-by: charlotte <61573792+charlotte-zhuang@users.noreply.github.com>
…t/Core-v4 into membershipFormBug
I changed code as requested, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check comments. PS it's nice to have a space before parentheses for non-functions like if ()
and before curly braces like function foo() {
The form isn't working either. Invalid password/email messages don't appear, and nothing happens when all input is valid.
I think it could help to simplify your name and account fields to have a checkRequirements
attribute that points to a function to check requirements. If requirements are met, return { pass: true }
. If requirements are not met, return { pass: false, message: <span>message here<span> }
. Or something like that.
Co-authored-by: charlotte <61573792+charlotte-zhuang@users.noreply.github.com>
Co-authored-by: charlotte <61573792+charlotte-zhuang@users.noreply.github.com>
Fix some bugs
There's addon and ifRequirementsNotMet, somebody before me did the addon part and I think it's meant to appear after the user click the submitted button, while ifRequirementsNotMet is for when user is filling out the form. I still don't get the 'checkRequirements' attribute part, like all of them point to a single function? |
1. Removed addon attribute 2. Changed when alert messages appear 3. Fixed password validation 4. Make password requirements clearer
1. Changed submit button to type="submit" 2. Added Form component 3. There's a bug in Firefox where you can't scroll when a password field is selected. This might not be our problem as changing the input type to text fixes the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I went and just made the changes that still needed to be made
Changes
- Fixed a few bugs
- Removed
addon
attribute from form - Changed invalid alerts to only show after user clicks submit
- Added more detailed password requirement feedback
- Wrapped the entire form in a
Form
component to allow users to submit with enter
Issues
- Firefox can't scroll when a password field is selected. Changing
type="text"
fixes the issue for some reason (but is undesirable). - Some links and buttons aren't in the tab order. Adding
tabindex="0"
androle="tablist", role="tab"
didn't change anything.
Pictures
thanks for improving the code, @charlotte-zhuang! |
@khainl1110 I think you can merge this PR. |
@charlotte-zhuang it said merging is blocked for me, can you or someone do it instead? Thanks |
Added messages to user if they left any input fields empty or invalid
Resolves #660