fix: Allow "spaces" in theme names while saving and display a more relevant error message - #15837
Conversation
dhruvikn
added
Bug
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Unable to find test scripts. Please add necessary tests to the PR. |
|
/ok-to-test sha=9a8e982 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2817356855. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2817356855. Click to view performance test results
|
somangshu
left a comment
somangshu
left a comment
There was a problem hiding this comment.
Apart from the comments, LGTM
| export const NAME_SPACE_ERROR = () => `Name must not have spaces`; | ||
| export const SPECIAL_CHARACTER_ERROR = () => `Name must be alphanumeric`; | ||
| export const APLHANUMERIC_HYPHEN_SLASH_SPACE_ERROR = () => | ||
| `Name must only contain alphanumeric characters, hyphen, slash, and space`; |
There was a problem hiding this comment.
@dilippitchika does the copy look ok to you?
cc @jnikhila
There was a problem hiding this comment.
@somangshu - looks good to me. Just one minor thing, if we can use something like "A name must begin with a letter and cannot start with a space" instead of "Name must not have spaces"- as it means that the name field cannot have spaces at all.
There was a problem hiding this comment.
@somangshu - On another note, why are there different error messages for different validations for the same field? Can we club them all to show a single error message? Adding detail and getting one after the other error message will frustrate users.
There was a problem hiding this comment.
@jnikhila the different validation messages are for different kinds of error, clubbing this mean the user will not have a pin pointed idea of what is doing wrong and also might have to jump into the debugger every time, AFAIK This change was made because the user did not have the a good idea about the error. I am not sure if in terms of UX this is a problem, @momcilo-appsmith @dilippitchika can comment on that better.
@dhruvikn for the other suggested change, we should create a new issue with the relevant details and take it up later. Can you do this?
Just one minor thing, if we can use something like "A name must begin with a letter and cannot start with a space" instead of "Name must not have spaces"- as it means that the name field cannot have spaces at all.
There was a problem hiding this comment.
Hi @jnikhila @somangshu. I think there is some confusion. Let me try to paint a clear picture. We have a file called messages.ts that has a lot of messages which are shown on particular conditions throughout Appsmith.
However, for this Theme Name field, there are only three messages used. These three messages are shown on different occasions. The messages for the Theme Name field are -
ERROR_MESSAGE_NAME_EMPTY = Please select a name
APLHANUMERIC_HYPHEN_SLASH_SPACE_ERROR = Name must only contain alphanumeric characters, hyphen, slash, and space
UNIQUE_NAME_ERROR = Name must be unique
Now, as for the NAME_SPACE_ERROR = Name must not have spaces, the author intended to say that "the name field cannot have spaces at all". Reference - #914
| import { | ||
| createMessage, | ||
| ERROR_MESSAGE_NAME_EMPTY, | ||
| SPECIAL_CHARACTER_ERROR, |
There was a problem hiding this comment.
@dhruvikn is this being used elsewhere? Why are we not using the same variable?
There was a problem hiding this comment.
No, the SPECIAL_CHARACTER_ERROR is not being used anywhere else. We can remove it entirely.
However, APLHANUMERIC_HYPHEN_SLASH_SPACE_ERROR seems a more suitable name for the message.
|
/ok-to-test sha=cadbf38 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2823100713. |
|
Tested this PR and working as expected |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2823100713. Click to view performance test results
|
|
/ok-to-test sha=f0610fe |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2834317606. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2834317606. Click to view performance test results
|
|
/ok-to-test sha=f7510b2 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2836814969. |
|
/ok-to-test sha=f7510b2 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2836985535. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2836985535. Click to view performance test results
|
|
/ok-to-test sha=68dd876 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2845078154. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2845078154. Click to view performance test results
|
4abba7e to
68dd876
Compare
|
/ok-to-test sha=a77f9b1 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2868611293. |
|
/ok-to-test sha=a77f9b1 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2874756591. |
1 similar comment
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2874756591. |
| if (/[^a-zA-Z0-9\-\/\ ]/.test(value)) { | ||
| isValid = false; | ||
| errorMessage = createMessage(SPECIAL_CHARACTER_ERROR); | ||
| errorMessage = createMessage(APLHANUMERIC_HYPHEN_SLASH_SPACE_ERROR); |
5 participants
Description
This PR intends to allow users to include "space(s)" in their theme name while creating/saving a new theme. Moreover, a new and more relevant error message is displayed which lets the user know what all is accepted in the theme name input.
Previously, while saving a new theme, we got an error if the name had any "space". However, Appsmith already has an in-house theme with a space in its name. 😄
Fixes #14840
Type of change
How Has This Been Tested?
Checklist: