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

BorderControl: Make fieldset and legend use optional #42611

Closed

Conversation

aaronrobertshaw
Copy link
Contributor

Related:

What?

Makes rendering the BorderControl as a fieldset with a legend for its label, optional.

Why?

The BorderControl is primarily used within the BorderBoxControl which itself should be a fieldset. This PR facilitates avoiding nested fieldsets.

Further context can be found here: #42351 (comment)

How?

  • Adds a new renderAsFieldset prop to the BorderControl
  • Updates the BorderControl to render using a fieldset/legend combo based on the new prop

Testing Instructions

  1. Checkout this PR and start up the Storybook: npm run storybook:dev
  2. Vist the BorderControl page
  3. Inspect the control and note by default its wrapper should be a div with label
  4. Toggle the renderAsFieldset prop to true i the controls
  5. Re-inspect the control, it should now be using a fieldset/legend combo. There should also be no visual change.

This facilitates avoiding nested fieldsets when used within a BorderBoxControl.
@aaronrobertshaw aaronrobertshaw added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Jul 22, 2022
@aaronrobertshaw aaronrobertshaw self-assigned this Jul 22, 2022
Copy link
Contributor

@glendaviesnz glendaviesnz left a comment

Choose a reason for hiding this comment

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

Worked as advertised for me

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for the quick iteration! I have a question about the labelling below; otherwise, this does what it says on the title 😄

<BorderLabel
label={ label }
hideLabelFromVision={ hideLabelFromVision }
as={ renderAsFieldset ? 'legend' : undefined }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking that, if this element isn't a legend, and if it's hidden from vision, it's essentially useless, because screen readers won't announce it. On the other hand, if this is part of a split control, we do need some form of accessible labelling, otherwise there's no way of telling which side is which.

I think ideally we wouldn't render BorderLabel at all unless it's a legend, and would instead pass the label value directly to the controls below, where it could be prepended to their existing labels. Would this be doable?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd tend to agree this should either render a fieldset + legend or nothing. Also, if there's a fieldset rendered, the legend (the label prop) should be required, as rendering an unnamed group is a bit pointless.

and would instead pass the label value directly to the controls below, where it could be prepended to their existing labels

Not sure that would be ideal, as concatenation in translatable strings is problematic for some languages and should be avoided.

@ciampo ciampo requested a review from mirka July 22, 2022 11:27
@ciampo
Copy link
Contributor

ciampo commented Jul 22, 2022

The code works as expected, although I'm not sure about introducing such a specific prop that highlights an implementation detail.

I'd also like to take a look at this issue from a more general point of view, as we're going to face this issue across many components (for example, soon when working on SliderControl and SliderNumberControl).

I opened #42630 to discuss the best approach moving forward.

) : (
<StyledLabel as="legend">{ label }</StyledLabel>
<StyledLabel { ...labelProps }>{ label }</StyledLabel>
Copy link
Contributor

Choose a reason for hiding this comment

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

To my understanding, this will render a stray <label> element, ie. a <label> that isn't associated with any field.

@afercia
Copy link
Contributor

afercia commented Jul 26, 2022

Overall, I'm not sure this component should be responsible for rendering a fieldset + legend. I'd agree with @ciampo that seems something that belongs to the implementation side. Should these kind of 'base' component only render the form controls? Thanks for creating #42630 to have a place for a broader discussion 👍

@aaronrobertshaw
Copy link
Contributor Author

Thank you everyone for the reviews and feedback. 👍

I'm not clear on what the next steps should be so am inclined to put this PR on hold until the discussion in #42630 unfolds. I'll happily revisit it once we decide on a path forward.

@aaronrobertshaw
Copy link
Contributor Author

aaronrobertshaw commented Mar 21, 2024

Closing this PR due to its age and lack of clear direction for a solution. It can be reopened if required.

The root issue (#42630) will remain open as it is still important to address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants