Skip to content

Conversation

@ad3872
Copy link
Contributor

@ad3872 ad3872 commented Jan 26, 2026

Summary of the changes

Refactored ic-time-input to remove the Date object so now it only shows the time as a string.

Related issue

#3873

Checklist

General

  • Changes to docs package checked and committed.
  • All acceptance criteria reviewed and met.

Testing

  • Relevant unit tests and visual regression tests added.
  • Playground stories in React Storybook up to date, with any prop changes and additions addressed.

System modes

  • Browser support tested (Chrome, Safari, Firefox and Edge).

doc changes for removal of date from the ic-time-input canary component
…mponent

Refactored the Date object logic so it handles time as a string to prevent the date from appreaing
in the console.
@CLAassistant
Copy link

CLAassistant commented Jan 26, 2026

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Contributor

Copy link
Contributor

@jd3267 jd3267 left a comment

Choose a reason for hiding this comment

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

I think in general this is a good effort and addresses the ticket however maybe over complicates it somewhat, resulting in some errors. The Date object should be kept allowing comparison of times (e.g in max and min time) rather than using the string. I've suggested a change which should output just the time part of the Date object

this.selectedTime = value;
if (!onlyPeriodChanged) {
this.icTimeChange.emit({
value: allSelected ? time : null,
Copy link
Contributor

@jd3267 jd3267 Jan 26, 2026

Choose a reason for hiding this comment

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

I think if you change this to value: allSelected && time ? time.toLocaleTimeString() : null, that should change return the time portion of the Date object

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.

4 participants