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

feat(numeral-date): update component to render as a fieldset element #7082

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

nuria1110
Copy link
Contributor

@nuria1110 nuria1110 commented Nov 20, 2024

Proposed behaviour

Updates NumeralDate's HTML to be rendered as a <fieldset> element.

Current behaviour

NumeralDate is rendered as a number of input elements within <div> wrappers, which has no semantic meaning.

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

  • Ensure existing props work as expected when used with new and legacy validation patterns.
  • There should be no significant visual differences, some small pixel changes to be expected.

LegendHelp is only used as a prop to render hint text with new validation, but does not render a
help icon with tooltip if used with the legacy validation.
@@ -100,7 +98,7 @@ For more information check our [Validations](../?path=/docs/documentation-valida

This is an example of `Checkbox` in a `CheckboxGroup` with validations passed as a string.

**Note:** The `legendHelp` tooltip will be rendered as "Hint text".
**Note:** The `legendHelp` will be rendered as "Hint text".
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The legendHelp prop never rendered a tooltip and is only ever used to render the "Hint Text" within a fieldset with the new validation pattern. Updated the docs to reflect this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good spot! 👍🏼

@Parsium Parsium self-requested a review November 21, 2024 09:18
Parsium
Parsium previously approved these changes Nov 21, 2024
required={required}
/>
</FormSpacingProvider>
<StyledDateLabel
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment: I wonder if this should be a label element so that when a user clicks on it the relevant input is focused

)}

<StyledNumeralDate onKeyDown={onKeyDown} data-component="numeral-date">
{dateFormat.map((datePart, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): It looks like some of the composition here is the same as above, I wonder if it would be possible to store some of this in a variable and pass it to the relevant wrappers depending on the status of the validation flag.

const computedInputs = (...)

  if (!validationRedesignOptIn) {
   return (
     <TooltipProvider helpAriaLabel={helpAriaLabel}>
        <StyledFieldset
          ...
          >
          {computedInputs()}
        </StyledFieldset>
     </TooltipProvider>
   )
 }
 
 return (
    <StyledFieldset
       ...
     >
        {computedInputs()}
    </StyledFieldset>

edleeks87
edleeks87 previously approved these changes Nov 25, 2024
Parsium
Parsium previously approved these changes Nov 25, 2024
Copy link
Contributor

@Parsium Parsium left a comment

Choose a reason for hiding this comment

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

Great work on this @nuria1110 👍🏼

@Parsium Parsium marked this pull request as ready for review November 25, 2024 10:19
@Parsium Parsium requested review from a team as code owners November 25, 2024 10:19
@harpalsingh
Copy link
Member

Reviewed this with a code look for semantic as well as testing voiceover in Safari, passing a11y review too.

@nuria1110 nuria1110 dismissed stale reviews from Parsium and edleeks87 via d967a77 December 2, 2024 12:51
@nuria1110 nuria1110 force-pushed the FE-6832 branch 2 times, most recently from 846ac88 to 261e89d Compare December 3, 2024 09:51
Adds `tooltipId` prop to provide an id for the legacy validation tooltip.
Updates NumeralDate component to render as a fieldset HTML element with a legend.
@nuria1110 nuria1110 merged commit db34a09 into master Dec 3, 2024
24 checks passed
@nuria1110 nuria1110 deleted the FE-6832 branch December 3, 2024 15:20
@carbonci
Copy link
Collaborator

carbonci commented Dec 3, 2024

🎉 This PR is included in version 144.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants