-
Notifications
You must be signed in to change notification settings - Fork 138
feat: accessibility improvements #363
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
base: master
Are you sure you want to change the base?
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(无则使用 React.useId 生成)
C->>H: 传递 id 与 headingLevel
H->>P: 将 id(可能附加索引)和 headingLevel 传给各 Panel
P->>W: 根据 headingLevel 包装 header
W-->>P: 返回处理后的 header
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/Collapse.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. Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
src/Collapse.tsx
Outdated
@@ -6,6 +6,7 @@ import useItems from './hooks/useItems'; | |||
import type { CollapseProps } from './interface'; | |||
import CollapsePanel from './Panel'; | |||
import pickAttrs from '@rc-component/util/lib/pickAttrs'; | |||
import { v4 as uuid4 } from 'uuid'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we try not to rely on additional third-party packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need to add this - the purpose was to create a default ID that is unique across multiple react applications that are rendered at the same time. However, if a consumer needs that, they can pass in an ID so I will go ahead and remove this package.
@yoyo837 I removed the uuid package, ready for re-review. |
@@ -71,6 +80,8 @@ const convertItemsToNodes = (items: ItemType[], props: Props) => { | |||
collapsible={mergeCollapsible} | |||
onItemClick={handleItemClick} | |||
destroyInactivePanel={mergeDestroyInactivePanel} | |||
headingLevel={headingLevel} | |||
id={id ? `${id}__item-${index}` : undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use rc-util
useId hooks in Panel. It support to use user pass id and fallback with ReactID if not have.
</div> | ||
{showArrow && iconNode} | ||
<span | ||
className={classNames(`${prefixCls}-title`, customizeClassNames?.title)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace this with HeadingElement instead of span directly. It's no need to have additional HeaderWrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Trace #362 |
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.
headingLevel
prop which will wrap the collapse header in a header elementid
prop which will override the default id mentioned aboveSummary by CodeRabbit
Style
新功能
Tests