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: Add unicode character support for default avatars #33882

Open
wants to merge 65 commits into
base: develop
Choose a base branch
from

Conversation

noobbbbb
Copy link
Contributor

@noobbbbb noobbbbb commented Nov 5, 2024

This PR allows non-alphanumeric characters to appear as the default avatar for users and channels. (but only for strings allowed in the settings)

Proposed changes (including videos or screenshots)

Issue(s)

#33602

Steps to test or reproduce

Further comments

@noobbbbb noobbbbb requested a review from a team as a code owner November 5, 2024 01:11
Copy link
Contributor

dionisio-bot bot commented Nov 5, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented Nov 5, 2024

⚠️ No Changeset found

Latest commit: 6ad48c4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@noobbbbb
Copy link
Contributor Author

noobbbbb commented Nov 5, 2024

Can someone please tell me...
In the “Run rossjrw/pr-preview-action@v1” step of the “ CI / deploy-preview (pull_request) ‘ stage, I get the error ’Error: Resource not accessible by integration”, How can I resolve this issue?

@sampaiodiego
Copy link
Member

Can someone please tell me... In the “Run rossjrw/pr-preview-action@v1” step of the “ CI / deploy-preview (pull_request) ‘ stage, I get the error ’Error: Resource not accessible by integration”, How can I resolve this issue?

don't worry about it.. this is not required to merge the PR, but the tests are..

also, do you mind explaining your use case? what character do you want to have used on avatars?

@noobbbbb
Copy link
Contributor Author

noobbbbb commented Nov 6, 2024

don't worry about it.. this is not required to merge the PR, but the tests are..

also, do you mind explaining your use case? what character do you want to have used on avatars?

Thank you for your support.
Regarding the characters we would like to use for the default avatar, specifically the Japanese string.

eg.)
Channel “公式” -> “公” is displayed.
User “山田 太郎” -> “山” is displayed.

In both cases, we would like to be able to display anything that can pass the validation patterns specified in “UTF8_User_Names_Validation” and “UTF8_Channel_Names_Validation”, rather than being limited to alphanumeric characters.

@noobbbbb
Copy link
Contributor Author

noobbbbb commented Nov 7, 2024

Could you help me again?

but the tests are...

Test UI (EE) / MongoDB 5.0 (2/5) - Alpine (Official) (pull_request)  Failing after 6m

I don't know why the error occurs.
What should I do?

@noobbbbb
Copy link
Contributor Author

noobbbbb commented Nov 8, 2024

It seems to have succeeded except for “CI / deploy-preview (pull_request)”.

@sampaiodiego
Could you review this?
And if everything is ok, could you merge this?

Copy link
Member

@gabriellsh gabriellsh left a comment

Choose a reason for hiding this comment

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

I think this PR should be labeled as feat, since it's changing a core feature. In this case you'll also need to add a changeset. It would be good to have a more descriptive title too, such as Add unicode character support for default avatars or something similar.

Please refer to our docs on informantion on adding changesets and pull request tags.

Can you also please update the tests in utils.spec.ts to contemplate these changes?

.toUpperCase();
const getFirstLetter = (name: string, regExp: RegExp) => {
const pattern = regExp || defaultPattern;
const sanitizedName = name.replace(/[&<>]/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

Please use dompurify to sanitize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed to use dompurify.

Comment on lines 140 to 142
return req.url.startsWith('/room')
? (settings.get('UTF8_Channel_Names_Validation') as string)
: (settings.get('UTF8_User_Names_Validation') as string);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be necessary. These settings validate username and room name when they are created, so roomOrUsername will be already respecting them. You'd also not need to check the URL in this case.

Was there a speicfic need to do this I might've missed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a futile consideration.
Changed to be concise and only perform sanitization process.

}

const fontSize = viewSize / 1.6;

return `<svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 ${viewSize} ${viewSize}\">\n<rect width=\"100%\" height=\"100%\" fill=\"${color}\"/>\n<text x=\"50%\" y=\"50%\" dy=\"0.36em\" text-anchor=\"middle\" pointer-events=\"none\" fill=\"#ffffff\" font-family=\"'Helvetica', 'Arial', 'Lucida Grande', 'sans-serif'\" font-size="${fontSize}">\n${initials}\n</text>\n</svg>`;
return `<svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 ${viewSize} ${viewSize}\">\n<rect width=\"100%\" height=\"100%\" fill=\"${color}\"/>\n<text x=\"50%\" y=\"50%\" dy=\"0.36em\" text-anchor=\"middle\" pointer-events=\"none\" fill=\"#ffffff\" font-family=\"'Helvetica', 'Arial Unicode MS', 'Noto Sans', 'Segoe UI', 'Lucida Grande', 'sans-serif'\" font-size="${fontSize}">\n${initials}\n</text>\n</svg>`;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how we handle typeface changes like these. I'll raise the concern internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also unnecessary.
I honestly can't remember why the change was made. I have reverted it back.

@noobbbbb noobbbbb changed the title chore: Change avatar initial extraction pattern feat: Add unicode character support for default avatars Nov 11, 2024
noobbbbb and others added 29 commits November 15, 2024 08:45
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.

4 participants