feat(time): align Time with FE-7233 audit requirements#8052
feat(time): align Time with FE-7233 audit requirements#8052tamas-sage wants to merge 11 commits into
Conversation
| pmText: () => "PM", | ||
| hoursLabelText: () => "Hrs.", | ||
| minutesLabelText: () => "Mins.", | ||
| hoursLabelText: () => "Hours", |
There was a problem hiding this comment.
praise: good idea just replacing the string with the correct one instead of deleting this translation entirely, as it would have been a breaking change if used in consumers code
| @@ -16,9 +16,10 @@ import { filterStyledSystemMarginProps } from "../../style/utils"; | |||
|
|
|||
| import Fieldset from "../../__internal__/fieldset"; | |||
There was a problem hiding this comment.
suggestion: I believe we should be using the new Fieldset component here via ../../__internal__/__next__/
This will automatically give you the updated tokens for the legend, legent hint, required asterisk etc.
| id: hoursInputId, | ||
| label: hoursLabel, | ||
| "aria-label": hoursAriaLabel, | ||
| "aria-label": hoursInputAriaLabel, |
There was a problem hiding this comment.
praise: hoursInputAriaLabel is definitely a better name here, makes it much clearer where the prop is being used
| ); | ||
|
|
||
| await checkAccessibility(page); | ||
| await checkAccessibility(page, undefined, "color-contrast"); |
There was a problem hiding this comment.
suggestion: I've tested this locally, and it appears to pass without the addition of this rule exception, same applies to the disabled test above so you can remove the colour contrast rule exception from there too if you'd like.
I believe since the test above was written aria-disabled has been added to elements with disabled styling which can't be natively disabled via the disabled attribute, so axe-core does not flag the component as having colour contrast issues 👍
| /** Override the default label text */ | ||
| label?: string; | ||
| /** Override the default aria-label text */ | ||
| /** Override the default aria-label text on the input */ |
There was a problem hiding this comment.
suggestion: if you're adding this to the prop description you should also add it to any other props within TimeInputProps for consistency sake
nuria1110
left a comment
nuria1110
left a comment
There was a problem hiding this comment.
Nice work on this, just a couple comments from me along with Tom's.
We will also need to deprecate label and inputHint and replace them with legend and legendHint, we will need to map them in the meantime so they still work.
legend={legend || label}
legendHint={legentHint || inputHint}
While aligning NumeralDate we found that is it not semantically correct to call it a label as its actually a legend so to be consistent we will have to do the same here, you can see how it was done here.
3 participants
Proposed behaviour
Updates the Time component to match FE-7233 audit requirements by replacing NumberInput with Textbox, using internal Label/Fieldset patterns, applying numeric input mode, and updating time field labels to full words (e.g. “Hours”, “Minutes”) across locales. It also keeps AM/PM toggle optional and aligns disabled/read-only label styling with the new token approach.
Current behaviour
The Time component previously used NumberInput and legacy label styling, with abbreviated secondary labels (e.g. “Hrs.” / “Mins.” in some locales), and did not fully align with the latest audit token/pattern updates.
Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions