-
Notifications
You must be signed in to change notification settings - Fork 151
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
fix(blade): chipgroup grid alignment #2536
Changes from 5 commits
39da608
67b9d5f
fb84e53
2481620
1e89e97
72efaeb
18838c7
464acc8
f280c95
98296fb
7c9803a
f3633f3
a818274
dc6fa5a
23ee578
f4652cb
bb40a3f
62f74c1
2a32ed1
a1f66f9
c02f1e9
1836fdc
8adc366
3443de8
1774421
833b335
4be16b6
bf73016
eee166f
29bfc15
aad91c2
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@razorpay/blade': patch | ||
--- | ||
|
||
fix(blade): allow consumers to align chips in chipgroup |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,5 +144,5 @@ const getStyledProps = (props: Record<string, any>): StyledPropsBlade => { | |
return removeUndefinedStyledProps(makeStyledProps(props)); | ||
}; | ||
|
||
export type { StyledPropsBlade }; | ||
export type { StyledPropsBlade, StyledPropsInputType }; | ||
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. where is this used? 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. removed |
||
export { getStyledProps, makeStyledProps, removeUndefinedStyledProps }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,7 @@ type OnChange = ({ | |
}) => void; | ||
|
||
const _Chip: React.ForwardRefRenderFunction<BladeElementRef, ChipProps> = ( | ||
{ isDisabled, value, children, icon: Icon, color, testID, _motionMeta, ...rest }, | ||
{ isDisabled, value, children, icon: Icon, color, testID, _motionMeta, width, ...rest }, | ||
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. we should expose |
||
ref, | ||
) => { | ||
const { theme } = useTheme(); | ||
|
@@ -201,6 +201,7 @@ const _Chip: React.ForwardRefRenderFunction<BladeElementRef, ChipProps> = ( | |
} | ||
height={makeSize(chipHeightTokens[_size])} | ||
gap="spacing.3" | ||
width={width} | ||
> | ||
{Icon ? ( | ||
<BaseBox display="flex"> | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -727,3 +727,80 @@ chipRef.parameters = { | |||||
exclude: ['icon'], | ||||||
}, | ||||||
}; | ||||||
|
||||||
export const MultiChipStory: StoryFn<typeof ChipGroupComponent> = () => { | ||||||
const chipArray = [ | ||||||
{ | ||||||
value: '100', | ||||||
label: '₹100', | ||||||
}, | ||||||
{ | ||||||
value: '500', | ||||||
label: '₹500', | ||||||
}, | ||||||
{ | ||||||
value: '1000', | ||||||
label: '₹1000', | ||||||
}, | ||||||
{ | ||||||
value: '2000', | ||||||
label: '₹2000', | ||||||
}, | ||||||
{ | ||||||
value: '5000', | ||||||
label: '₹5000', | ||||||
}, | ||||||
{ | ||||||
value: '10000', | ||||||
label: '₹10000', | ||||||
}, | ||||||
{ | ||||||
value: '20000', | ||||||
label: '₹20000', | ||||||
}, | ||||||
{ | ||||||
value: '50000', | ||||||
label: '₹50000', | ||||||
}, | ||||||
{ | ||||||
value: '100000', | ||||||
label: '₹100000', | ||||||
}, | ||||||
{ | ||||||
value: '200000', | ||||||
label: '₹200000', | ||||||
}, | ||||||
]; | ||||||
|
||||||
return ( | ||||||
<Box gap="spacing.3" display="flex" flexDirection="column"> | ||||||
<ChipGroupComponent | ||||||
selectionType="single" | ||||||
label="Select a gift card with value (without custom chipGroupContainerLayout)" | ||||||
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.
Suggested change
what is chipGroupContainerLayout? 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.
|
||||||
> | ||||||
{chipArray.map((chip, index) => ( | ||||||
<ChipComponent key={index} value={chip.value}> | ||||||
{chip.label} | ||||||
</ChipComponent> | ||||||
))} | ||||||
</ChipGroupComponent> | ||||||
<ChipGroupComponent | ||||||
selectionType="single" | ||||||
label="Select a gift card with value (custom chipGroupContainerLayout)" | ||||||
> | ||||||
<Box | ||||||
display="grid" | ||||||
gridTemplateColumns="repeat(3, minmax(0,120px))" | ||||||
gridTemplateRows="repeat(3, minmax(0,30px))" | ||||||
gap="spacing.3" | ||||||
> | ||||||
{chipArray.map((chip, index) => ( | ||||||
<ChipComponent key={index} value={chip.value} width="120px"> | ||||||
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. 🤔 We shouldn't need to add explicit pixel values in ChipComponent. ideally width=100% or flex={1} should work, instead of hard coding 120px like so:
I would assume this would work and the chip should automatically stretch/flex to available space. Not to mention explicit pixel values will break the layout if there is no space: ![]() |
||||||
{chip.label} | ||||||
</ChipComponent> | ||||||
))} | ||||||
</Box> | ||||||
</ChipGroupComponent> | ||||||
</Box> | ||||||
); | ||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import { chipGroupGapTokens, chipGroupLabelSizeTokens } from './chipTokens'; | |
import { ChipGroupProvider } from './ChipGroupContext'; | ||
import { useChipGroup } from './useChipGroup'; | ||
import type { ChipGroupProps } from './types'; | ||
import { getLayOutProps } from './utils'; | ||
import BaseBox from '~components/Box/BaseBox'; | ||
import { FormHint, FormLabel } from '~components/Form'; | ||
import { SelectorGroupField } from '~components/Form/Selector/SelectorGroupField'; | ||
|
@@ -12,6 +13,7 @@ import { Text } from '~components/Typography'; | |
import type { BladeElementRef } from '~utils/types'; | ||
import { throwBladeError } from '~utils/logger'; | ||
import { makeAnalyticsAttribute } from '~utils/makeAnalyticsAttribute'; | ||
import { getComponentId } from '~utils/isValidAllowedChildren'; | ||
|
||
const _ChipGroup = ( | ||
{ | ||
|
@@ -68,6 +70,24 @@ const _ChipGroup = ( | |
}); | ||
} | ||
} | ||
const isWrapperLayoutControlled = | ||
typeof children === 'object' && getComponentId(children) === 'Box'; | ||
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.
This code i shared, should just work without doing any of these checks/conditionals. 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. Yes, but I think we should add a wrapper and check for styles since consumers can pass props like background color or margin and padding, which should not be supported. 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. We should not do arbitrary checks if it's absolutely not necessary. If they add background etc anyways it will look odd. Your above code will fail in many cases like so: const SomeCustomWrapperBox = styled(Box)``
<ChipGroup>
<SomeCustomWrapperBox>
...
</SomeCustomWrapperBox>
</ChipGroup>
`` |
||
const getChipGroupWrapperStyles = (): Record<string, unknown> => { | ||
if (isWrapperLayoutControlled) { | ||
return { | ||
...(React.isValidElement(children) ? getLayOutProps(children.props) : {}), | ||
}; | ||
} | ||
return {}; | ||
}; | ||
const getChipWrapperChildren = (): React.ReactNode => { | ||
if (isWrapperLayoutControlled) { | ||
if (React.isValidElement(children) && children.props) { | ||
return children.props.children; | ||
} | ||
} | ||
return children; | ||
}; | ||
|
||
return ( | ||
<ChipGroupProvider value={contextValue}> | ||
|
@@ -96,8 +116,13 @@ const _ChipGroup = ( | |
<VisuallyHidden> | ||
<Text>{accessibilityLabel}</Text> | ||
</VisuallyHidden> | ||
<BaseBox display="flex" flexDirection="row" flexWrap="wrap"> | ||
{React.Children.map(children, (child, index) => { | ||
<BaseBox | ||
display="flex" | ||
flexDirection="row" | ||
flexWrap="wrap" | ||
{...(isWrapperLayoutControlled ? getChipGroupWrapperStyles() : {})} | ||
> | ||
{React.Children.map(getChipWrapperChildren(), (child, index) => { | ||
return ( | ||
<BaseBox | ||
key={index} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
// ref - /blade/packages/blade/src/components/Box/styledProps/getStyledProps.ts | ||
tewarig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// since here the use case is too specific that's why makeLayout Prop is inside Chip. | ||
|
||
import type { ChipGroupContainerLayoutProps } from './types'; | ||
import { removeUndefinedStyledProps } from '~components/Box/styledProps'; | ||
import type { KeysRequired } from '~utils/types'; | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
type StyledPropsInputType = Record<string, any>; | ||
|
||
const makeStyledProps = ( | ||
props: StyledPropsInputType, | ||
): KeysRequired<ChipGroupContainerLayoutProps> => { | ||
return { | ||
display: props.display, | ||
gridColumn: props.gridColumn, | ||
gridRow: props.gridRow, | ||
gridArea: props.gridArea, | ||
alignSelf: props.alignSelf, | ||
justifySelf: props.justifySelf, | ||
placeSelf: props.placeSelf, | ||
order: props.order, | ||
gridColumnStart: props.gridColumnStart, | ||
gridColumnEnd: props.gridColumnEnd, | ||
alignContent: props.alignContent, | ||
alignItems: props.alignItems, | ||
columnGap: props.columnGap, | ||
gap: props.gap, | ||
gridAutoColumns: props.gridAutoColumns, | ||
gridAutoFlow: props.gridAutoFlow, | ||
gridAutoRows: props.gridAutoRows, | ||
gridTemplateAreas: props.gridTemplateAreas, | ||
grid: props.grid, | ||
gridTemplate: props.gridTemplate, | ||
gridTemplateColumns: props.gridTemplateColumns, | ||
gridTemplateRows: props.gridTemplateRows, | ||
justifyContent: props.justifyContent, | ||
justifyItems: props.justifyItems, | ||
rowGap: props.rowGap, | ||
flexBasis: props.flexBasis, | ||
flexDirection: props.flexDirection, | ||
flexGrow: props.flexGrow, | ||
flexShrink: props.flexShrink, | ||
flexWrap: props.flexWrap, | ||
flex: props.flex, | ||
placeItems: props.placeItems, | ||
gridRowEnd: props.gridRowEnd, | ||
gridRowStart: props.gridRowStart, | ||
}; | ||
}; | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const getLayOutProps = (props: Record<string, any>): ChipGroupContainerLayoutProps => { | ||
return removeUndefinedStyledProps(makeStyledProps(props)); | ||
}; | ||
|
||
export { getLayOutProps }; |
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.
where is this used?
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.
removed