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.

Sign up for GitHub

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

Jump to bottom

TimePicker input verification #1626

Open
gohde opened this issue Jan 13, 2023 · 4 comments · May be fixed by #1657
Open

TimePicker input verification #1626

gohde opened this issue Jan 13, 2023 · 4 comments · May be fixed by #1657
Labels
enhancement New feature or request flagship Question for @carbon-design-system/carbon good first issue Good for newcomers

Comments

@gohde
Copy link

gohde commented Jan 13, 2023

It would be very convenient to have something like the code below being part of the TimePicker component.

  function processTimeString(time: string): string {

    // Check for invalid characters.
    if(!/^[0-9:]*$/.test(time)){
      return "";
    }

    // Reject input that is too long or short.
    // Return empty string here to avoid : insertion.
    if(time.length > 5 || time.length == 0){
      return "";
    }

    // Regular expression to check if time is in hh:mm format.
    const timeFormat = /^([0-1]?[0-9]|2[0-3]):[0-5][0-9]$/;

    // Check if time is in hh:mm format.
    if (!timeFormat.test(time)) {
      // Add missing colon if only hours are provided.
      if (time.length === 2) {
        time = time + ":00";
      } else if (time.length === 1) {
        time = "0" + time + ":00";
      } else if (time.indexOf(':') === -1) {
        time = time.slice(0, 2) + ":" + time.slice(2);
      }
      // Check if hour is between 0 and 23.
      const hour = Number(time.slice(0, 2));
      if (hour < 0 || hour > 23) {
        return "";
      }
      // Check if minute is between 0 and 59.
      const minute = Number(time.slice(3, 5));
      if (minute < 0 || minute > 59) {
        return "";
      }
    }
    return time;
  }
@theetrain
Copy link
Collaborator

theetrain commented Jan 13, 2023 •
edited
Loading

Thanks for the suggestion! It looks like the supplied function performs input masking, and I couldn't find information on Carbon's official stance on input masking. It appears that input masking should be suggested, but not enacted as part of an input's helperText—which TimePicker seems to be missing.

It's implied that formatting user input while typing causes screen reader issues, but is acceptable on blur. If you're interested in having input masking while typing be supported by Carbon components, I suggest opening an issue in the flagship repository https://github.com/carbon-design-system/carbon/

I think formatting on blur should be an acceptable enhancement now since DatePicker does that today.

References:

@theetrain theetrain added enhancement New feature or request flagship Question for @carbon-design-system/carbon labels Jan 13, 2023
@gohde
Copy link
Author

gohde commented Jan 13, 2023

It is not meant to run onKeyUp, I run the above function onChange hooked up to the input reference of the component, it only needs to run when the user is done inputting and has actually made a change. This is to make it consistent with the DatePicker which I have next to it in the form. Although DatePicker runs onBlur I think.

@gohde
Copy link
Author

gohde commented Jan 13, 2023

From a UX perspective I find onChange better for form validation. It gives the user a chance to do things before nagging her or him to fix stuff. I prefer then to run all validators on submission to catch any un-changed required fields that the user didn't fill out.

@theetrain
Copy link
Collaborator

I agree, running this onChange sounds good to me; and your UX opinion aligns with me and Carbon guidelines: https://carbondesignsystem.com/patterns/forms-pattern/#behavior

@theetrain theetrain added the good first issue Good for newcomers label Jan 13, 2023
@mustafa-kapadia1483 mustafa-kapadia1483 linked a pull request Feb 12, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request flagship Question for @carbon-design-system/carbon good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add input verification mustafa-kapadia1483/carbon-components-svelte
2 participants
@gohde @theetrain