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

fix: channel and community naming #2302 #2584

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

richardtorres314
Copy link
Contributor

Pull Request Checklist

  • I have linked this PR to a related GitHub issue.
  • I have added a description of the change (and Github issue number, if any) to the root CHANGELOG.md.

(Optional) Mobile checklist

Please ensure you completed the following checks if you did any changes to the mobile package:

  • I have run e2e tests for mobile
  • I have updated base screenshots for visual regression tests

Copy link
Collaborator

@adrastaea adrastaea left a comment

Choose a reason for hiding this comment

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

I have stylistic qualms with this change. Mostly related to allowing whitespaces which I think make for a bad experience telling apart channels, typing references to them, and displaying them in the UI. I think fully pulling out pattern matching for channels and communities is the wrong move. I found this article for supporting international characters which might be worth testing, but I also don't know if this is something we want to support outside of the community name for the sake of users easily referencing channels. I think that in certain resolutions it may be hard to tell certain diacritical marks apart which would make it difficult to search or reference them. @holmesworcester could overrule me there though

@@ -90,7 +90,7 @@ describe('Add new channel', () => {
const alice = await factory.create<ReturnType<typeof identity.actions.addNewIdentity>['payload']>('Identity', {
nickname: 'alice',
})
const channelName = { input: 'my-Super Channel ', output: 'my-super-channel-' }
const channelName = { input: 'my-Super Channel ', output: 'my-Super Channel ' }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that allowing spaces in channel names makes sense for a couple reasons. It's impossible to tell how many spaces are in a channel name when referencing it. It looks like a bug in the UI when a channel name starts with white spaces. Also, the industry standard is to not allow capitals or spaces in channel names.

@@ -15,10 +15,6 @@ export const communityNameField = (name = 'name'): FieldData => {
value: 20,
message: CommunityNameErrors.NameTooLong,
},
pattern: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's worth still disallowing leading and trailing whitespace for the sake of aesthetics. There are also options for regexing to include diacritical marks if we want to continue to disallow special characters, but support international alphabets.

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.

2 participants