Skip to content

Add GBP (£) to Symbol List #2146

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

Closed

Conversation

richard-bridgeman
Copy link

Added the GBP £ pound sign in to Symbol List

Copy link
Contributor

@pano9000 pano9000 left a comment

Choose a reason for hiding this comment

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

Hi,

thanks for the PR. :-)
A) The change you made is not a valid RegExp though, you would need to add the symbol inside the "range" expression here:

so from:
/^[-#!$@%^&*()_+|~=`{}\[\]:";'<>?,.\/ ]$£/;
you would need to change to
/^[-#!$£@%^&*()_+|~=`{}\[\]:";'<>?,.\/ ]$/

However, regardless of that error:
the question would still be, if adding the GBP symbol here makes sense or not - I will have a look at this later this evening and provide some feedback :-)

Thanks!

@pano9000
Copy link
Contributor

this PR belongs to issue #2145

@WikiRik
Copy link
Member

WikiRik commented Jan 11, 2023

However, regardless of that error: the question would still be, if adding the GBP symbol here makes sense or not - I will have a look at this later this evening and provide some feedback :-)

The previous addition to the symbol list did not include any clarification on which symbols are supposed to be in that list so I think this addition is fine.

Apart from fixing the RegExp it would also be nice if a new test case could be added using £

@pano9000
Copy link
Contributor

The previous addition to the symbol list did not include any clarification on which symbols are supposed to be in that list so I think this addition is fine.

after checking isStrongPassword's history and PRs, I agree - the change from this PR seems fine, with the current state of isStrongPassword.

(I will likely create a new issue about it though, as I don't really think the current state is really that ideal -> but that is a separate topic :-))

@profnandaa
Copy link
Member

Sorry, closing as duplicate of #2148

@profnandaa profnandaa closed this Jan 20, 2023
@profnandaa profnandaa added the duplicate Duplicate issue label Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate Duplicate issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants