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 #11362: Workspace Name Character Limit #14260

Closed
wants to merge 1 commit into from
Closed

fix #11362: Workspace Name Character Limit #14260

wants to merge 1 commit into from

Conversation

poypoyan
Copy link
Contributor

@poypoyan poypoyan commented Nov 2, 2022

Resolves: #11362

Just added an extra condition in newworkspacemodel.cpp. Need review for the error message.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

Just added an extra condition in newworkspacemodel.cpp. Need review for the error message.
@@ -106,6 +106,11 @@ void NewWorkspaceModel::validateWorkspaceName()
.arg(m_workspaceName);
return;
}

if (m_workspaceName.size() > 24) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As @Tantacrul said #11362 (comment)

I suggest a cap of 24 characters. A user should not be able to enter more than that in the text field.

We should just ignore character input without showing an error message.

@bkunda
Copy link

bkunda commented Nov 2, 2022

Thanks @poypoyan for working on this!

My comments on this are:

  • We shouldn't show an error message when the text input field is empty. Just keep the "Select" button disabled.
  • We should allow an empty space after a character (and again, not show the error message "xxx cannot be used as a workspace name"). It's entirely plausible that a user might accidentally add a space after their workspace name, and this error message doesn't let them know why their name can't be used. If it's absolutely impossible for technical reasons to allow a space at the end of a workspace name, then we at least need a more helpful error message (E.g. "Please delete the space at the end of your workspace name"). But preferably, the space should be allowed and there should be no need for this error message.
  • We should allow more than 24 characters. I would suggest perhaps 48 instead.

This demo video is rather long (about 4:00), but you can see how confused I got 😅
https://www.dropbox.com/s/b71cpm6dy9wh4qq/workspace-error-message.mov?dl=0

Thank you so much again!

@Tantacrul
Copy link
Contributor

One comment @bkunda - I was suggesting a lower character limit so that the UI doesn't end up really wide:
image

@bkunda
Copy link

bkunda commented Nov 3, 2022

One comment @bkunda - I was suggesting a lower character limit so that the UI doesn't end up really wide: image

I don't disagree, but I think that 24 characters is a little too constrictive.

Even 40 characters feels a little more relaxed:

Screenshot 2022-11-03 at 10 40 46 am

Screenshot 2022-11-03 at 10 43 47 am

@Tantacrul
Copy link
Contributor

Tantacrul commented Nov 3, 2022

We haven't met in the middle here so I'll look weak in the eyes of the community if I give way to you. The only way forward is to double down, so I'm insisting on a character limit of 10.

(OK, 40's fine)

@RomanPudashkin
Copy link
Contributor

fixed in #14288

@poypoyan poypoyan deleted the 11362-workspacecharlim branch November 8, 2022 01:22
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.

[MU4 Issue] [Workspaces] Workspace Name Character Limit
5 participants