feat: accessibility improvements #363
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Walkthrough此次变更主要包含对 Collapse 组件及其子组件（Panel、PanelContent）的增强改造，同时对样式文件和测试用例进行了相应更新。CSS 中扩展了 header 的选择器以支持包裹结构；组件中新增了通过 Changes
Sequence Diagram(s)sequenceDiagram
participant U as 用户
participant C as Collapse组件
participant H as useItems钩子
participant P as Panel组件
participant W as HeaderWrapper
U->>C: 请求渲染 Collapse
C->>C: 检查是否提供 id（无则使用 useId 生成）
C->>H: 传递 id 与 headingLevel
H->>P: 将 id（可能附加索引）和 headingLevel 传给各 Panel
P->>W: 根据 headingLevel 包装 header
W-->>P: 返回处理后的 header
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/PanelContent.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. src/Panel.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. src/hooks/useItems.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@yoyo837 I removed the uuid package, ready for re-review. |
| </div> | ||
| {showArrow && iconNode} | ||
| <span | ||
| className={classNames(`${prefixCls}-title`, customizeClassNames?.title)} |
There was a problem hiding this comment.
replace this with HeadingElement instead of span directly. It's no need to have additional HeaderWrapper.
There was a problem hiding this comment.
I can change HeaderWrapper, but replacing the span would not satisfy the a11y standards for collapse / accordion components. The heading element should wrap the header button, which is where HeaderWrapper is in this PR. Would you prefer it to be a div that is always present around the button, instead of conditionally rendered, but it only would have the heading role and aria-level when the prop is defined?
There was a problem hiding this comment.
OK. So you can add on the rc-collapse-header directly.
There was a problem hiding this comment.
Update - the rc-collapse-header element gets the button role, so to satisfy the a11y standards for the header, I changed to a div element that always wraps rc-collapse-header and will conditionally have the heading role and aria-level props set.
There was a problem hiding this comment.
@zombieJ Can you please review my latest changes with my previous comment in mind?
|
Trace #362 |
|
@jon-cullison is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
| @@ -34,8 +35,11 @@ const Collapse = React.forwardRef<HTMLDivElement, CollapseProps>((props, ref) => | |||
| items, | |||
| classNames: customizeClassNames, | |||
| styles, | |||
| headingLevel, | |||
There was a problem hiding this comment.
Currently we can simply use h3 instead of configuable headingLevel
|
Would be conflicted with #362 |
| style={styles?.title} | ||
| {...(collapsible === 'header' ? collapsibleProps : {})} | ||
| <div {...resetProps} ref={ref} className={collapsePanelClassNames} id={id}> | ||
| <div |
src/Panel.tsx
Outdated
| <div | ||
| {...headerProps} | ||
| id={id ? `${id}__header` : undefined} | ||
| aria-controls={id ? `${id}__content` : undefined} |
There was a problem hiding this comment.
this should move into collapsibleProps
src/Panel.tsx
Outdated
| @@ -110,6 +123,8 @@ const CollapsePanel = React.forwardRef<HTMLDivElement, CollapsePanelProps>((prop | |||
| return ( | |||
| <PanelContent | |||
| ref={motionRef} | |||
| id={id ? `${id}__content` : undefined} | |||
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
+ Coverage 99.15% 99.16% +0.01%
==========================================
Files 5 5
Lines 118 120 +2
Branches 43 44 +1
==========================================
+ Hits 117 119 +2
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Implements 2/3 of the proposed changes here:
ant-design/ant-design#53203
I did not implement the panel role change because there is already some logic around setting the panel role to "tabpanel" when in accordion mode, and the panel role = "region" is optional for a11y purposes.
headingLevelprop which will wrap the collapse header in a header elementidprop which will override the default id mentioned aboveSummary by CodeRabbit
Style
新功能
Tests