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

[core] feat(Section): a11y improvements, titleRenderer prop #6506

Merged

Conversation

bvandercar-vt
Copy link
Contributor

@bvandercar-vt bvandercar-vt commented Nov 3, 2023

Improve a11y on the Section component by adding various aria props.

  • Includes tests

@bvandercar-vt bvandercar-vt changed the title feat(Section): a11y improvements feat(Section): a11y improvements, allow custom tag for title Nov 3, 2023
Comment on lines 115 to 118
/**
* @default H6
*/
titleTagName?: keyof JSX.IntrinsicElements | React.FC<React.AllHTMLAttributes<HTMLElement>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we limit this to only HTMLHeadingElement function components, so that users are encouraged to use Blueprint's heading components? also I think the JSDoc needs to be improved

Suggested change
/**
* @default H6
*/
titleTagName?: keyof JSX.IntrinsicElements | React.FC<React.AllHTMLAttributes<HTMLElement>>;
/**
* The heading component to use when rendering the title, typically one of `H1`, `H2,
* `H3`, `H4`, `H5`, or `H6` from `@blueprintjs/core`.
*
* @default H6
*/
titleHeadingComponent?: React.FC<React.AllHTMLAttributes<HTMLHeadingElement>>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the purpose for this change, for our application at least, is specifically to not have it be inside of a heading element. This is so that we can pass an element to the title prop that has a heading element within it. In our case, here is our goal:

title={
      <div>
        <H2 id={titleId}>{title}</H2>
        {titleRightElement}
      </div>
    }

in which case we'd want to set titleTagName to div, since it will contain the H2.

So I see 3 options for us to achieve our goal above:

  1. We implement titleHeadingComponent the way you said, and then add a prop titleRightElement.
  2. We implement titleHeadingComponent the way you said, and on our end we just add a ts ignore so that we can pass a div component instead of a heading component.
  3. We scrap this change and add a titleProps prop which gets spread to the H6 so that we can pass our div component to title and set titleProps={{ role: 'presentation'}}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, how about a render prop API instead?

type SectionTitleProps = Pick<React.HTMLAttributes<HTMLElement>, "className" | "id">;
/**
 * Optional title renderer function. It is recommended to render a Blueprint `<H6>` element as part of the title.
 *
 * @default H6
 */
renderTitle?: React.FC<SectionTitleProps>;

Note: this type is informed by a recent fix we had to make for a similar render prop API in #6521

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright I take back my last comment, your first solution would work. Kept that solution. Renamed to renderTitle instead of titleHeadingComponent though. Up for whatever you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

upon further review, I decided that titleRenderer is a better name. I pushed a commit with that update

packages/core/src/components/section/section.tsx Outdated Show resolved Hide resolved
const isHeaderRightContainerVisible = rightElement != null || collapsible;

const sectionId = uniqueId("section");
const sectionTitleId = title ? uniqueId("section-title") : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const sectionTitleId = title ? uniqueId("section-title") : undefined;
const sectionTitleId = title != null ? uniqueId("section-title") : undefined;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line 174 has logic

 {title && (
                <div

so we would want this logic to be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could change that to title != null too though. Depends whether you want empty string to create the title element.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, good point, let's keep what you have

@adidahiya adidahiya changed the title feat(Section): a11y improvements, allow custom tag for title [core] feat(Section): a11y improvements, titleRenderer prop Nov 15, 2023
@adidahiya adidahiya enabled auto-merge (squash) November 15, 2023 13:46
@adidahiya
Copy link
Contributor

Sorry about the unrelated lint job error in CI; I will fix that on develop.

Your docs preview build looks good

image

@adidahiya adidahiya merged commit df48fb2 into palantir:develop Nov 15, 2023
11 of 12 checks passed
@bvandercar-vt bvandercar-vt deleted the bvandercar/section-a11y-improvements branch May 9, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants