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

Extend slugify function #8740

Merged

Conversation

AyushMainali123
Copy link
Contributor

@AyushMainali123 AyushMainali123 commented May 7, 2023

What does this PR do?

Closes #7718

The slugify function appears to have limited functionality, which led to issue #7718. Additionally, I observed that the function does not trim multiple spaces in the slug.

For example:
Previously, ayush mainali was updated to ayush------mainali.
test test test πŸ‘©πŸ½πŸ‘¨πŸ½ ζΌ’ would be converted to test-test-test-----------, which is quite confusing.

Now,
ayush mainali will be converted to ayush-mainali,
test test test πŸ‘©πŸ½πŸ‘¨πŸ½ ζΌ’ will be converted to test-test-test-πŸ‘©πŸ½πŸ‘¨πŸ½-ζΌ’

I didn't use something like:
/[^a-zA-Z0-9-\p{Emoji}]/gu because, it couldn't properly slugify combined emoji like πŸ‘ͺ . It would return something like πŸ‘¨-πŸ‘©-πŸ‘¦ when slugified.

Fixes # (7718 )
https://user-images.githubusercontent.com/49358949/236681331-41a7cd2d-7334-4291-a5ff-30ebc8bc8936.mp4

Environment: Staging(main branch) / Production

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How should this be tested?

  • Register a user and add a username with a string including multiple spaces and emojis. The output will be a properly slugified username.
  • Update username, the text will be properly slugified here too.

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works

I have checked test suites using yarn run test (yarn test-e2e was testing against 0 test suites in my local) .

Test
Weekday tests β€Ί fn: weekdayNames β€Ί should return the weekday names for a given locale and format was failing in the production branch, so it isn't addressed in my PR.

Except for that test case, all other test case has passed in the PR I submitted.

@vercel
Copy link

vercel bot commented May 7, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
cal βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback May 17, 2023 0:58am
ui βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback May 17, 2023 0:58am

@vercel
Copy link

vercel bot commented May 7, 2023

@AyushMainali123 is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2023

πŸ“¦ Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. πŸ€–

This PR introduced no changes to the JavaScript bundle! πŸ™Œ

@PeerRich PeerRich requested a review from zomars May 7, 2023 23:06
Copy link
Member

@sean-brydon sean-brydon left a comment

Choose a reason for hiding this comment

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

Tested - LGTM

Will wait for a few others to test tho as this is core code.

@zomars @keithwillcode <3

@sean-brydon
Copy link
Member

@PeerRich @baileypumfleet are we actually wanting to support emojis in usernames πŸ€”

Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Definitively an improvement. Thank you for your contribution πŸ™

@zomars
Copy link
Member

zomars commented May 12, 2023

we actually wanting to support emojis in usernames

We could discuss this in a follow up

@zomars zomars enabled auto-merge May 12, 2023 16:56
@zomars zomars added this pull request to the merge queue May 12, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 12, 2023
@keithwillcode keithwillcode added this pull request to the merge queue May 17, 2023
Merged via the queue into calcom:main with commit 4b356dd May 17, 2023
21 checks passed
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.

When using EMOJI as username Avatar breaks πŸ’€
4 participants