chore: Fix GroupHeading props + some other cleanup
#296
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e434952:
|
📊 Package size report 1%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
| @@ -98,7 +98,7 @@ const Control = < | |||
| data-focus-visible={isFocused ? true : undefined} | |||
| data-invalid={isInvalid ? true : undefined} | |||
| data-disabled={isDisabled ? true : undefined} | |||
| aria-readonly={isReadOnly ? true : undefined} | |||
| data-readonly={isReadOnly ? true : undefined} | |||
There was a problem hiding this comment.
This is not actually an input component, and does not have an aria role for an input, so it makes more sense to use the data-readonly prop, as this is just to trigger the readonly input styles from the Charka theme
| */ | ||
| export const DownChevron = (props: IconProps) => ( | ||
| <Icon {...props}> | ||
| <Icon role="presentation" focusable="false" aria-hidden="true" {...props}> |
There was a problem hiding this comment.
I'm not sure if these aria roles matter, but they're on Chakra's select component, so I decided to add them here too: https://github.com/chakra-ui/chakra-ui/blob/61f965a/packages/components/select/src/select.tsx#L168-L179
| > | ||
| {children || <DownChevron sx={iconSx} />} | ||
| {children || <DownChevron sx={downChevronSx} />} |
There was a problem hiding this comment.
No real changes here, just moved some types around and renamed the variables to be more consistent with the other components in the package
| paddingX: horizontalPaddingOptions[size], | ||
| paddingY: verticalPaddingOptions[size], |
There was a problem hiding this comment.
After some testing in my theme branch, I realized that the single padding style was not being overridden by user defined padding values. I believe splitting them up gives less specificity, allowing the user defined styles to override them.
| display: "flex", | ||
| alignItems: "center", | ||
| width: "100%", | ||
| textAlign: "start", | ||
| fontSize: size, | ||
| padding: paddings[size], | ||
| ...(isFocused && menuItemStyles._focus), |
There was a problem hiding this comment.
I realized this would be better implemented by adding the data-focus attribute to the menu item, triggering the style in a way more in line with Chakra's theme system.
| @@ -438,8 +448,9 @@ export const Option = < | |||
| )} | |||
| sx={sx} | |||
| ref={innerRef} | |||
| data-disabled={isDisabled ? true : undefined} | |||
There was a problem hiding this comment.
I realized there was no reason to use both data-disabled and aria-disabled. The data- props Chakra looks at for styling seem to be more of a fallback if the aria- props aren't applicable to be applied to a component.
| | (string & {}); | ||
| export type Variant = ThemeTypings["components"]["Input"]["variants"]; | ||
|
|
||
| export type ColorScheme = ThemeTypings["colorSchemes"]; |
There was a problem hiding this comment.
I realized I could get intellisense autocompletion for this by using Chakra's built in type for it. I also changed the other Variant types to use Chakra's built in types for these components instead of defining them manually.
closes #295
This PR is in response to #295. I had already found the fix for this issue, so this PR just implements it. The solution was just to implement some code I had missed from the original
GroupHeadingcomponent.It also implements a few other cleanup changes:
ThemeTypingsinterface.Here's a recreation of the demo linked in the issue, see below for the updated sandbox: https://codesandbox.io/s/msxddg?file=/ChakraAccordion.tsx