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

Validate values in the cell and table properties UI #6131

Closed
oleq opened this issue Jan 23, 2020 · 7 comments · Fixed by ckeditor/ckeditor5-table#238
Closed

Validate values in the cell and table properties UI #6131

oleq opened this issue Jan 23, 2020 · 7 comments · Fixed by ckeditor/ckeditor5-table#238
Assignees
Labels
domain:accessibility This issue reports an accessibility problem. domain:ui/ux This issue reports a problem related to UI or UX. package:table type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@oleq
Copy link
Member

oleq commented Jan 23, 2020

📝 Provide a description of the new feature

A follow-up of #6049.

ATM correct or wrong (broken, incomplete, etc.), they go straight to the model. We need validators that will display errors next to the fields and prevent that.

ATM changes go to the model on keydown (or dropdown value change) which may affect collaboration in the wrong way (too many changes). However, when validation will work, this problem will (probably, see my comment below) be negligible.

Some cases to consider:

  • what to do with "Save" when some fields are invalid?
  • what to do with "Cancel" when some fields are invalid?
  • what to do if someone clicked outside the UI when some fields are invalid?
  • where to display messages? (especially border fields) some balloons? UI research needed.

Related topic: form creator/generator.


If you'd like to see this feature implemented, add a 👍 reaction to this post.

@oleq oleq added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:table domain:ui/ux This issue reports a problem related to UI or UX. labels Jan 23, 2020
@oleq oleq added this to the iteration 29 milestone Jan 23, 2020
@oleq
Copy link
Member Author

oleq commented Jan 23, 2020

An afterthought:

  • if changes are applied to the editor model on keydown (input):

    • it is great because the preview is live in the editing,
    • the validators must work with a delay. Why? Because otherwise, this nasty UX will happen:
      1. type "r",
      2. validator displays an error and highlights the field red "This color is invalid",
      3. type "e",
      4. validator displays an error and highlights the field red "This color is invalid",
      5. type "d",
      6. the validator message and color disappear
      7. transaction to the model via the command

    See, if the validation is immediate, users will see invalid fields all the time, they'll drown in the see of red.

  • if changes are applied on blur:

    • we can validate at that point
    • until then the field is not marked as invalid (a good UX)
    • we lose the live preview in the editing :P

So what we probably need is a more advanced (also time–consuming) approach where:

  • validators react on the keydown (preventing transaction to the model),
  • but the error in the UI is debounced (~1s maybe).

@jodator
Copy link
Contributor

jodator commented Jan 23, 2020

  • we lose the live preview in the editing :P

There's enter alongisde 'blur' event (cons: discoverability).

So what we probably need is a more advanced (also time–consuming) approach where:

It looks like OK approach in terms of no model spamming with wrong values and the error will be shown eventually. It also looks like less work from what we have ATM if I'm right.

@oleq
Copy link
Member Author

oleq commented Jan 23, 2020

There's enter alongisde 'blur' event (cons: discoverability).

I'd rather expect Enter to submit the entire form. But... who knows?

@mlewand
Copy link
Contributor

mlewand commented Jan 23, 2020

So what we probably need is a more advanced (also time–consuming) approach where:

@oleq I don't have the full context here, but is it possible to debounce its appearance with CSS only? Just as if you'd have animation that does nothing first second and does a soft fadein after this time?

This way it could be all based on CSS alone, because as you continue to type and the error is gone, DOM is changed and this CSS effect would no longer be relevant?

@oleq
Copy link
Member Author

oleq commented Jan 23, 2020

It can be postponed visually in CSS, but the screen reader will read it right away. So this is an issue, could be screen reader users will just flip out (imagine a new error after each keypress :P).

Also, I'm not sure how will it work when the error changes. TBH, I can't picture all the possible scenarios in my head, so some research will be needed first.

But yeah, it could be we're onto something here @mlewand!

@mlewand
Copy link
Contributor

mlewand commented Jan 23, 2020

@oleq Oh, you're right I didn't took accessibility into consideration, so we need to be careful with DOM change timing - unless screen readers do throttle these notifications internally. Would require checking.

@oleq
Copy link
Member Author

oleq commented Feb 4, 2020

Notes from the F2F meeting:

  • Validate on key press,
  • Let's debounce ("1", "1p", "1px" case),
  • Pin the error balloon directly to the field when error.
    • No button to open the error, it's instant (but debounced).
  • No infos/hints, we expect users to discover what is right and what is wrong.
  • Develop a new private component in ckeditor5-table (input with balloon errors).
    • Maybe it'll become a public one after a trial period.
  • "Save" button should be disabled as long as any field is invalid.
  • "Cancel" does what is does now. It just closes the UI. No prompt even if some fields are invalid.
  • Clicking outside the form hides the UI (works like Cancel), so no prompt.

jodator added a commit to ckeditor/ckeditor5-theme-lark that referenced this issue Feb 10, 2020
Feature: Added styles for rich error messages in the table and table cell properties forms (see ckeditor/ckeditor5#6131).
jodator added a commit to ckeditor/ckeditor5-table that referenced this issue Feb 10, 2020
Internal: Table and table cell property form fields should be validated. Closes ckeditor/ckeditor5#6131.
@Reinmar Reinmar added the domain:accessibility This issue reports an accessibility problem. label Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:accessibility This issue reports an accessibility problem. domain:ui/ux This issue reports a problem related to UI or UX. package:table type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants