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

feat(ui-library): made ids optional and provided fallback #641

Closed

Conversation

ChristianHoffmannS2
Copy link
Collaborator

No description provided.

// the bitwise OR replaces Math.floor which is much faster,
// but will fail on very large numbers, which is not the case here
if (validFirstChar) {
return characters.charAt((Math.random() * validFirstCharactersLength) | 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I assume that the first character can never be a number which is why you have this check in place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, exactly.- according to specs, it has to start with letters.

there are more characters allowed here actually (dash for example) but its fine like this i guess

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

code was from stackover flow bad had that issue, i just optimized it and fixed it

Copy link
Contributor

@davidken91 davidken91 left a comment

Choose a reason for hiding this comment

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

Do you ever foresee a scenario where the user would want to provide their own id?

@thrbnhrtmnn thrbnhrtmnn deleted the optional-ids-with-crypto-fallback branch November 28, 2023 15:32
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