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

Added Correct form validation #5087

Merged
merged 1 commit into from
Mar 20, 2019
Merged

Conversation

ananyaarun
Copy link
Member

@ananyaarun ananyaarun commented Mar 15, 2019

Fixes #3646

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • PR is descriptively titled 📑
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

Some GIFs to show the working

  • Case with Null user name and password followed by correct username and password

login

  • Case to show wrong username and password

login1

Thanks!

@plotsbot
Copy link
Collaborator

2 Messages
📖 @ananyaarun Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progress’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

@ananyaarun
Copy link
Member Author

@gauravano , i have added the required clause for username and password. Hence the form is never submitted with null values and the wrong alert never shows up. Kindly review :) Thanks !

@grvsachdeva
Copy link
Member

Hey @ananyaarun , can't see null value part in the Gif

@ananyaarun
Copy link
Member Author

Hey @gauravano , Sorry the first part of the first gif was unclear. My bad!!
Here is a better gif.
new

Copy link
Member

@grvsachdeva grvsachdeva left a comment

Choose a reason for hiding this comment

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

Nice work @ananyaarun !

@jywarren
Copy link
Member

Hmm, is there any way to make it appear more urgently, like with a red highlight? If not, I'm fine merging this! I just don't want people to get confused as to why the button isn't working, and maybe not noticing the messages. Thanks!

@ananyaarun
Copy link
Member Author

@jywarren , There is a pop up that says "pls enter this field" when login doesnt work. So i dont think the user will be confused ?

@jywarren
Copy link
Member

Ok, I think this works then! I was just worried maybe it wasn't noticable enough but you're probably right and we can do some user testing to be sure. Thanks, merging now!!!

@jywarren jywarren merged commit 81b0963 into publiclab:master Mar 20, 2019
cesswairimu pushed a commit to cesswairimu/plots2 that referenced this pull request Mar 21, 2019
icarito pushed a commit to icarito/plots2 that referenced this pull request Apr 9, 2019
SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants