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

Provide more specific errors for incorrect Local path during creating a site #901

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

nightnei
Copy link
Contributor

@nightnei nightnei commented Feb 6, 2025

Related issues

Testing Instructions

"Site name" change

  1. Click on "Add site" button in the left bottom corner
  2. Provide "test" as site name and create site
  3. Click on "Add site" again
  4. Provide "test" as site name again
  5. I didn't think about the fact that we create directory based on the site name, so for me it was confusing seeing some error and I didn't get what happened at a glance. I think that such error will be more specific to show the error and at the same time explain that site name and path are connected, so the user should either chose another site name or keep it but manually select directory name for this site.
BEFORE AFTER
Screenshot 2025-02-06 at 15 03 58 Screenshot 2025-02-06 at 14 26 28

Adding new site with existing folder

  1. Create a site in Studio
  2. Click on "Add site"
  3. Try to create a new site with the folder which is already connected to another site in Studio
  4. I think, showing the next error, would be:
    4.1 more specific - menthion that the directory is already connected specifically tosome site in Studio
    4.2 concise - user selected some directory to connect to Studio, we don't know was their intention to create new site or add some existing WP folder, so saying "empty directory to create site" looks confusing if they wonted to add existed WP site.
BEFORE AFTER
Screenshot 2025-02-06 at 15 07 24 Screenshot 2025-02-06 at 15 08 31

@nightnei nightnei requested a review from a team February 6, 2025 15:09
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

I confirm the PR updates the error message accordingly, but I wonder if having the expanded message is enough, and it wouldn't increase the complexity in our codebase.

My suggestion is to leave the long error message for both cases:

The directory is already associated with another site. Please choose a different "Site name" or adjust the "Local path"

Comment on lines +127 to +128
errorPathIsNotAvailable = __(
'This directory is already connected to another site in Studio.'
Copy link
Member

Choose a reason for hiding this comment

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

Case when the user enters a path so they are already modifying and seeing the output. 👍 We don't need much information.

Copy link
Contributor

Choose a reason for hiding this comment

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

It leads to inconsistent flow:

  1. A user has the site FooBar in the directory of the same name
  2. The user tries to add a new site with the name 'FooBar' which leads to using a duplicate directory, seeing 'The directory is already associated with another Studio site. Please choose a different site name or a custom local path.' error.
  3. User clicks directory selector, and clicks 'FooBar' directory, and error changes to 'This directory is already connected to another site in Studio.'.

It would make sense to include instruction in this case, too:

The directory is already associated with another Studio site. Please choose a different custom local path.

);
} else {
errorPathIsNotAvailable = __(
'The directory is already associated with another site. Please choose a different "Site name" or adjust the "Local path".'
Copy link
Member

Choose a reason for hiding this comment

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

The user modifies the name and the inferred path conflicts with another site. The users won't see this error until they expand the toggle.

@wojtekn
Copy link
Contributor

wojtekn commented Feb 10, 2025

@sejas @nightnei I would propose to:

  • add 'Studio' to make it even clearer what site it means
  • simplify wording to say what parts the user can change instead of referring to just field labels

So it could say:

The directory is already associated with another Studio site. Please choose a different site name or a custom local path.

Also, when testing that, I noticed another issue related to case insensitivity. If users create a site in a FooBar directory and then try to add a site using the FooBar name, they won't get a correct error message:

Screenshot 2025-02-10 at 11 17 13 Screenshot 2025-02-10 at 11 17 23

@nightnei
Copy link
Contributor Author

nightnei commented Feb 10, 2025

The directory is already associated with another Studio site. Please choose a different site name or a custom local path.

@wojtekn it makes sense, but for the first case of Testing instruction. Why I made another wording for the second case (Adding new site with existing folder) is - changing site name won't have any effect on the path in this case, since it's another flow which should not do changes to path if it manually was changed. That's why I decided that it's two different flows and for the second case it's better to show some simple and specific error.

We don't add a lot of lines of code and difficult conditions, everything looks simple regarding the code, so keeping 2 different errors will give more benefits for UI and no difficulties for codebase.

WDYT?

@nightnei
Copy link
Contributor Author

nightnei commented Feb 10, 2025

Also, when testing that, I noticed another issue related to case insensitivity. If users create a site in a FooBar directory and then try to add a site using the FooBar name, they won't get a correct error message:

@wojtekn good catch 👍 Reproduced and fixed.
just wonder your opinion - I decide that it's better to male Studio case sensitive for both Windows and MacOS. I think that it's very edge case if somebody want to have "Site name" and "site name" at the same time, and also it's better to keep consistency behaviour of Studio in this case and such approach maybe even simplify maintenance for us and investigating bugs. Are you okay with going with case sensitivity for both of them?

@wojtekn
Copy link
Contributor

wojtekn commented Feb 11, 2025

@wojtekn it makes sense, but for the first case of Testing instruction. Why I made another wording for the second case (Adding new site with existing folder) is - changing site name won't have any effect on the path in this case, since it's another flow which should not do changes to path if it manually was changed. That's why I decided that it's two different flows and for the second case it's better to show some simple and specific error.

@nightnei sounds reasonable, the text is clear.

just wonder your opinion - I decide that it's better to male Studio case sensitive for both Windows and MacOS. I think that it's very edge case if somebody want to have "Site name" and "site name" at the same time, and also it's better to keep consistency behaviour of Studio in this case and such approach maybe even simplify maintenance for us and investigating bugs. Are you okay with going with case sensitivity for both of them?

I'm unsure if Studio should explicitly support a case-sensitive or case-insensitive system. Shouldn't it be agnostic and rely on whatever fs.pathExists returns? However, we can tackle that under a separate issue.

@sejas
Copy link
Member

sejas commented Feb 11, 2025

just wonder your opinion - I decide that it's better to male Studio case sensitive for both Windows and MacOS. I think that it's very edge case if somebody want to have "Site name" and "site name" at the same time, and also it's better to keep consistency behaviour of Studio in this case and such approach maybe even simplify maintenance for us and investigating bugs. Are you okay with going with case sensitivity for both of them?

I'm unsure if Studio should explicitly support a case-sensitive or case-insensitive system. Shouldn't it be agnostic and rely on whatever fs.pathExists returns? However, we can tackle that under a separate issue.

I vote in favor of making Studio case insensitive on both platforms. We shouldn't allow two folders have the same name regarding the case of the letters. So FolderName should conflict with foldername

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.

3 participants