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

Display uniqueness requirement for name and disable save if not met #4164

Merged
merged 8 commits into from
Dec 14, 2023

Conversation

CarolineDenis
Copy link
Contributor

Fixes #4150

@CarolineDenis
Copy link
Contributor Author

CarolineDenis commented Oct 26, 2023

Do we want to keep the warning also on the list behind the dialog or now that we have it in the form we're good?
@grantfitzsimmons
Screenshot 2023-10-26 at 11 27 34 AM

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

this is a good first try, but can you try to improve it:
could you try using the useValidation hook to create a validation error on the name field when there are duplicates?

benefits:

  • the error would be connected to the field that caused it - making it even easier to understand how to fix it
  • if user tries to submit the form, the invalid field will get focus again and error message will be displayed shown again automatically
  • better screen reader/accessibility support

--

as far as having the validation behind this dialog, some thoughts:

  • take a look if something in the code would go terribly wrong if name is not unique (i.e terribly wrong means a crash, or loss of data; not terribly wrong is formatter that is used is not the one user was expecting)
  • we are adding the UI for editing this, but in the past people might have hand-edited these files, so there could be mistakes that we have to catch an present as soon as possible
  • on the other hand, it would be annoying to have to fix all of the mistakes before you can continue on with your work just because some test formatter you are no longer using has a duplicate name
  • having the same check in both places is redundant (more code, and also a bit distracting when the error appears in both places)

given that UI is not letting user create new mistakes, that alone is good enough and forcing them to fix previous mistakes may be too forcefull.

@grantfitzsimmons
Copy link
Member

Do we want to keep the warning also on the list behind the dialog or now that we have it in the form we're good?

If the error appears in the dialog, we can remove it from behind. Our goal is to prevent the user from closing the dialog until the issue is resolved, right? In that case, we might want to not let the user close the dialog until it's resolved.

image

@CarolineDenis CarolineDenis added this to the 7.9.3 milestone Oct 30, 2023
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

looks great!

isn't it cool how few lines of code it takes to build a fully-accessible and user-friendly validation, that is also quite robust?

@CarolineDenis CarolineDenis marked this pull request as ready for review November 1, 2023 14:01
@CarolineDenis CarolineDenis requested review from a team and melton-jason November 1, 2023 14:02
Copy link
Member

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

You can bypass this by clicking on the space outside of the dialog

Screen.Recording.2023-11-01.at.9.26.17.AM.mov

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

@grantfitzsimmons

You can bypass this by clicking on the space outside of the dialog

I think forbidding closing the dialog is worse UX than accidentally creating two formatters with the same name and thus seeing wrong one being used on the form

What if user wants to briefly look at something behind the dialog? (i.e some other formatter, or a notification they have just received)

The role of validation in this dialog in my opinion is not bulletproof data integrity, but guiding the user towards right choices (guiding, not holding under the gun)


but I am not a user so feel free to pick either way

@CarolineDenis
Copy link
Contributor Author

@grantfitzsimmons

You can bypass this by clicking on the space outside of the dialog

I think forbidding closing the dialog is worse UX than accidentally creating two formatters with the same name and thus seeing wrong one being used on the form

What if user wants to briefly look at something behind the dialog? (i.e some other formatter, or a notification they have just received)

The role of validation in this dialog in my opinion is not bulletproof data integrity, but guiding the user towards right choices (guiding, not holding under the gun)

but I am not a user so feel free to pick either way

The user could drag the dialog on the side otherwise?

@grantfitzsimmons
Copy link
Member

grantfitzsimmons commented Nov 2, 2023

@maxpatiiuk If we disallow saving with duplicate view names, that would be acceptable. Otherwise, it's preferable to require them to resolve the issue before dismissing it.

@CarolineDenis
Copy link
Contributor Author

CarolineDenis commented Nov 2, 2023

@maxpatiiuk If we disallow saving with duplicate view names, that would be acceptable. Otherwise, it's preferable to require them to resolve the issue before dismissing it.

I suggest we keep it like this then and if the user needs to see what's behind, he would move the dialog

@maxpatiiuk
Copy link
Member

sounds good, just pointing out that the ability to bypass validation might be a feature and not a bug

@emenslin emenslin self-requested a review December 13, 2023 20:40
Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

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

This works and you can no longer close the popup if it has the same name either by using the close button or by clicking out of it. Can still move the popup though if the user did need to see something behind it.

@CarolineDenis CarolineDenis merged commit 902dba5 into xml-editor Dec 14, 2023
9 checks passed
@CarolineDenis CarolineDenis deleted the issue-4150 branch December 14, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

xml-editor: Formatter Name uniqueness requirement not shown in the dialog
5 participants