Repository navigation
feat: Enabled support for more than 4 columns for key-value pair #5081
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: main
Are you sure you want to change the base?
Changes from all commits
beca849
95a388f
0aad064
39d69e9
b040f53
e452f31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| import React from 'react'; | ||
| import clsx from 'clsx'; | ||
|
|
||
| import { useUniqueId, warnOnce } from '@cloudscape-design/component-toolkit/internal'; | ||
| import { useUniqueId } from '@cloudscape-design/component-toolkit/internal'; | ||
|
|
||
| import Box from '../box/internal'; | ||
| import ColumnLayout from '../column-layout/internal'; | ||
|
|
@@ -48,18 +48,9 @@ const InternalKeyValuePairs = React.forwardRef( | |
| ariaLabelledby, | ||
| minColumnWidth, | ||
| ...rest | ||
| }: KeyValuePairsProps & Required<Pick<KeyValuePairsProps, 'columns'>>, | ||
| }: KeyValuePairsProps & Required<Pick<KeyValuePairsProps, 'columns' | 'minColumnWidth'>>, | ||
| ref: React.Ref<HTMLDivElement> | ||
| ) => { | ||
| const MAX_COLUMNS = 4; | ||
|
|
||
| if (columns > MAX_COLUMNS) { | ||
| warnOnce( | ||
| 'Key-value pairs', | ||
| `\`columns\` (${columns}) must be <= ${MAX_COLUMNS}. Using ${MAX_COLUMNS} as default.` | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <LinkDefaultVariantContext.Provider value={{ defaultVariant: 'primary' }}> | ||
| <div | ||
|
|
@@ -70,15 +61,11 @@ const InternalKeyValuePairs = React.forwardRef( | |
| ref={ref} | ||
| > | ||
| {/* | ||
| minColumnWidth={150} is set to use FlexibleColumnLayout which has only 1 nested div wrapper for column items, | ||
| otherwise GridColumnLayout will be used, which has 2 nested div, therefore it is not a11y compatible for dl -> dt/dd relationship | ||
| A minColumnWidth is always passed (KeyValuePairs defaults it to 150) so that ColumnLayout | ||
| picks FlexibleColumnLayout, which wraps each column item in a single div. GridColumnLayout | ||
| wraps in two, which breaks the dl -> dt/dd relationship a11y relies on. | ||
| */} | ||
| <ColumnLayout | ||
| __tagOverride="dl" | ||
| columns={Math.min(columns, MAX_COLUMNS)} | ||
| variant="text-grid" | ||
| minColumnWidth={minColumnWidth} | ||
| > | ||
| <ColumnLayout __tagOverride="dl" columns={columns} variant="text-grid" minColumnWidth={minColumnWidth}> | ||
|
vikaskale2722 marked this conversation as resolved.
Resolved
vikaskale2722 marked this conversation as resolved.
Resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if a consumer passes
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will internally use GridColumnLayout (instead of FlexibleColumnLayout which it uses with minColumnWidth) and beyond 4 columns it will cause unexpected behavior. But passing 0 as a input to the column width does not make sense. |
||
| {items.map((pair, index) => { | ||
| if (pair.type === 'group') { | ||
| return ( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.