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

Change validation rules in new participant form #377

Merged

Conversation

Marce1ina
Copy link
Contributor

Story / Bug id:

#309

Description:

In new participant form:

  • removed characters type validation for "First name" and "Last name",
  • created custom validators for "GitHub username" and "Slack nick".

Migrations:

N/A

New imports / dependencies:

N/A

What tests do I need to run to validate this change:

Manual check of front-end form validation as follows:

  • "First name" should accept any character,
  • "Last name" should accept any charater,
  • "Github username" should accept latin alphabet letters, as well as numbers and -, with the constraint that username cannot start or end with -,
  • "Slack nick" should accept latin alphabet letters, as well as polish diacritical characters żźćńółęąś, numbers and ,.';-_/()[]{}.

@Marce1ina Marce1ina force-pushed the new-participant-form-validation branch from 4e4f6cf to 0e109d9 Compare April 26, 2021 06:42
Copy link
Contributor

@OtisRed OtisRed left a comment

Choose a reason for hiding this comment

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

I pulled the brunch, tried to spoil it, but it seems to work well. From my point of view it solves all the problems mentioned in the task so I leave approve but I would love for sb (@w1stler, @stanislawK, @jacekkalbarczyk, @kristhina) more fluent with regex to check the two experssions in custom validators at the bottom - before we merge it.

Comment on lines +189 to +192
import {
slackNickValidator,
gitHubUsernameValidator
} from '../helpers/validation';
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a neat trick to get around it - kudos!


export const slackNickValidator = helpers.regex(
'slackNickValidator',
/^[0-9A-Za-zżźćńółęąśŻŹĆĄŚĘŁÓŃ,.';\-_/()[\]{}]*$/
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate to be THAT guy but I would use: ąćęłńóśźżĄĆĘŁŃÓŚŹŻ - just for the sake of my OCD (and readability).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


export const gitHubUsernameValidator = helpers.regex(
'gitHubUsernameValidator',
/^[0-9A-Za-z][0-9A-Za-z-]*[0-9A-Za-z]$/
Copy link
Contributor

@stanislawK stanislawK May 5, 2021

Choose a reason for hiding this comment

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

Whenever You want match the character - literally with regex, it is safer to escape it and use "\-" instead of "-".

Copy link
Contributor Author

@Marce1ina Marce1ina May 9, 2021

Choose a reason for hiding this comment

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

@stanislawK I have removed it as it causes lint failure: error: Unnecessary escape character: \- (no-useless-escape) at src/helpers/validation.js:10:26 and it seems fair, please check https://stackoverflow.com/questions/9589074/regex-should-hyphens-be-escaped for reference.

@Marce1ina Marce1ina force-pushed the new-participant-form-validation branch from 6afaddc to 4fb4acd Compare May 9, 2021 10:02
@stanislawK stanislawK merged commit fde5ec5 into CodeForPoznan:master May 12, 2021
Marce1ina added a commit to Marce1ina/codeforpoznan.pl_v3 that referenced this pull request Jan 5, 2022
* Change validation rules in new participant form

* Change chars order in slackNickValidator
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