Skip to content
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.

Sign up for GitHub

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

chore: Fix GroupHeading props + some other cleanup #296

Merged
merged 8 commits into from
Oct 27, 2023
Merged

chore: Fix GroupHeading props + some other cleanup #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5ca9987
Simplify theme typings
csandman Oct 26, 2023
342f849
Update doc comment links
csandman Oct 26, 2023
19837d1
Add other aria props missing from DownChevron
csandman Oct 26, 2023
08226e6
Unify variable names and types
csandman Oct 26, 2023
6b87a85
Update one more doc comment
csandman Oct 26, 2023
09a5339
Fix GroupHeading props
csandman Oct 26, 2023
8ddad82
Clean up all aria props and base styles on them
csandman Oct 26, 2023
e434952
Add cursor styles to match old styles (from role="button")
csandman Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions src/chakra-components/control.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

>
{children}
</Box>
Expand Down Expand Up @@ -139,10 +139,11 @@ export const IndicatorSeparator = <
/**
* Borrowed from the `@chakra-ui/icons` package to prevent needing it as a dependency
*
* @see {@link https://github.com/chakra-ui/chakra-ui/blob/main/packages/icons/src/ChevronDown.tsx}
* @see {@link https://github.com/chakra-ui/chakra-ui/blob/61f965a/packages/components/icons/src/ChevronDown.tsx}
* @see {@link https://github.com/chakra-ui/chakra-ui/blob/61f965a/packages/components/select/src/select.tsx#L168-L179}
*/
export const DownChevron = (props: IconProps) => (
<Icon {...props}>
<Icon role="presentation" focusable="false" aria-hidden="true" {...props}>
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<path
fill="currentColor"
d="M16.59 8.59L12 13.17 7.41 8.59 6 10l6 6 6-6z"
Expand Down Expand Up @@ -187,7 +188,7 @@ export const DropdownIndicator = <
};
const iconSize = iconSizes[size];

const initialSx: SystemStyleObject = {
const initialDropdownIndicatorSx: SystemStyleObject = {
...inputStyles.addon,
display: "flex",
alignItems: "center",
Expand All @@ -205,17 +206,17 @@ export const DropdownIndicator = <
cursor: "inherit",
}),
};
const sx = chakraStyles?.dropdownIndicator
? chakraStyles.dropdownIndicator(initialSx, props)
: initialSx;
const dropdownIndicatorSx = chakraStyles?.dropdownIndicator
? chakraStyles.dropdownIndicator(initialDropdownIndicatorSx, props)
: initialDropdownIndicatorSx;

const initialIconStyles = {
const initialDownChevronSx: SystemStyleObject = {
height: "1em",
width: "1em",
};
const iconSx: SystemStyleObject = chakraStyles?.downChevron
? chakraStyles.downChevron(initialIconStyles, props)
: initialIconStyles;
const downChevronSx = chakraStyles?.downChevron
? chakraStyles.downChevron(initialDownChevronSx, props)
: initialDownChevronSx;

return (
<Box
Expand All @@ -227,17 +228,17 @@ export const DropdownIndicator = <
},
className
)}
sx={sx}
sx={dropdownIndicatorSx}
>
{children || <DownChevron sx={iconSx} />}
{children || <DownChevron sx={downChevronSx} />}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No real changes here, just moved some types around and renamed the variables to be more consistent with the other components in the package

</Box>
);
};

/**
* Borrowed from Chakra UI source
*
* @see {@link https://github.com/chakra-ui/chakra-ui/blob/13c6d2e08b61e179773be4722bb81173dd599306/packages/close-button/src/close-button.tsx#L14}
* @see {@link https://github.com/chakra-ui/chakra-ui/blob/61f965a/packages/components/close-button/src/close-button.tsx#L12-L21}
*/
export const CrossIcon = (props: IconProps) => (
<Icon focusable="false" aria-hidden {...props}>
Expand Down
4 changes: 1 addition & 3 deletions src/chakra-components/input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const Input = <
className,
cx,
value,
selectProps: { chakraStyles, isReadOnly, isRequired },
selectProps: { chakraStyles, isReadOnly },
} = props;
const { innerRef, isDisabled, isHidden, inputClassName, ...innerProps } =
cleanCommonProps(props);
Expand Down Expand Up @@ -76,8 +76,6 @@ const Input = <
sx={inputSx}
disabled={isDisabled}
readOnly={isReadOnly ? true : undefined}
aria-readonly={isReadOnly ? true : undefined}
aria-required={isRequired ? true : undefined}
{...innerProps}
/>
</Box>
Expand Down
65 changes: 38 additions & 27 deletions src/chakra-components/menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import type {
OptionProps,
} from "react-select";
import type { SizeProps, ThemeObject } from "../types";
import { useSize } from "../utils";
import { cleanCommonProps, useSize } from "../utils";

const alignToControl = (placement: CoercedMenuPlacement) => {
const placementToCSSProp = { bottom: "top", top: "bottom" };
Expand Down Expand Up @@ -118,6 +118,7 @@ export const MenuList = <

return (
<Box
role="listbox"
{...innerProps}
className={cx(
{
Expand Down Expand Up @@ -259,7 +260,10 @@ export const Group = <

const { chakraStyles } = selectProps;

const sx = chakraStyles?.group ? chakraStyles.group({}, props) : {};
const initialSx: SystemStyleObject = {};
const sx = chakraStyles?.group
? chakraStyles.group(initialSx, props)
: initialSx;

return (
<Box {...innerProps} className={cx({ group: true }, className)} sx={sx}>
Expand Down Expand Up @@ -288,11 +292,12 @@ export const GroupHeading = <
const {
cx,
className,
children,
// eslint-disable-next-line deprecation/deprecation
selectProps: { chakraStyles, size: sizeProp, hasStickyGroupHeaders },
} = props;

const { data, ...innerProps } = cleanCommonProps(props);

const menuStyles = useMultiStyleConfig("Menu");

const size = useSize(sizeProp);
Expand Down Expand Up @@ -325,9 +330,11 @@ export const GroupHeading = <
: initialSx;

return (
<Box className={cx({ "group-heading": true }, className)} sx={sx}>
{children}
</Box>
<Box
{...innerProps}
className={cx({ "group-heading": true }, className)}
sx={sx}
/>
);
};

Expand Down Expand Up @@ -371,20 +378,24 @@ export const Option = <
},
} = props;

const size = useSize(sizeProp);

const menuItemStyles: ThemeObject = useMultiStyleConfig("Menu").item;

const paddings: SizeProps = {
sm: "0.3rem 0.6rem",
md: "0.4rem 0.8rem",
lg: "0.5rem 1rem",
const size = useSize(sizeProp);
const horizontalPaddingOptions: SizeProps = {
sm: "0.6rem",
md: "0.8rem",
lg: "1rem",
};
const verticalPaddingOptions: SizeProps = {
sm: "0.3rem",
md: "0.4rem",
lg: "0.5rem",
};

/**
* Use the same selected color as the border of the select component
* Use the same selected color as the border/shadow of the select/input components
*
* @see {@link https://github.com/chakra-ui/chakra-ui/blob/13c6d2e08b61e179773be4722bb81173dd599306/packages/theme/src/components/input.ts#L73}
* @see {@link https://github.com/chakra-ui/chakra-ui/blob/61f965a/packages/components/theme/src/components/input.ts#L92-L93}
*/
const selectedBg = useColorModeValue(
`${selectedOptionColorScheme}.500`,
Expand All @@ -394,39 +405,38 @@ export const Option = <

// Don't create exta space for the checkmark if using a multi select with
// options that dissapear when they're selected
const showCheckIcon: boolean =
const showCheckIcon =
selectedOptionStyle === "check" &&
(!isMulti || hideSelectedOptions === false);

const shouldHighlight: boolean =
selectedOptionStyle === "color" && isSelected;
const shouldHighlight = selectedOptionStyle === "color";

const initialSx: SystemStyleObject = {
...menuItemStyles,
cursor: "pointer",
display: "flex",
alignItems: "center",
width: "100%",
textAlign: "start",
fontSize: size,
padding: paddings[size],
...(isFocused && menuItemStyles._focus),
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

paddingX: horizontalPaddingOptions[size],
paddingY: verticalPaddingOptions[size],
Comment on lines +422 to +423
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

...(shouldHighlight && {
bg: selectedBg,
color: selectedColor,
_active: { bg: selectedBg },
_selected: {
bg: selectedBg,
color: selectedColor,
_active: { bg: selectedBg },
},
}),
...(isDisabled && menuItemStyles._disabled),
...(isDisabled && { _active: {} }),
};

const sx = chakraStyles?.option
? chakraStyles.option(initialSx, props)
: initialSx;

return (
<Box
role="option"
{...innerProps}
role="button"
className={cx(
{
option: true,
Expand All @@ -438,8 +448,9 @@ export const Option = <
)}
sx={sx}
ref={innerRef}
data-disabled={isDisabled ? true : undefined}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

data-focus={isFocused ? true : undefined}
aria-disabled={isDisabled ? true : undefined}
aria-selected={isSelected}
>
{showCheckIcon && (
<MenuIcon
Expand Down
1 change: 1 addition & 0 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export type {
Size,
TagVariant,
SelectedOptionStyle,
ColorScheme,
StylesFunction,
ChakraStylesConfig,
OptionBase,
Expand Down
14 changes: 9 additions & 5 deletions src/module-augmentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { SystemStyleObject } from "@chakra-ui/system";
import type { GroupBase, StylesConfig, ThemeConfig } from "react-select";
import type {
ChakraStylesConfig,
ColorScheme,
SelectedOptionStyle,
SizeProp,
TagVariant,
Expand Down Expand Up @@ -68,11 +69,14 @@ declare module "react-select/base" {
/**
* If true, the form control will be required. This has 2 side effects:
*
* - The `FormLabel` will show a required indicator
* - The form element (e.g, Input) will have `aria-required` set to true
* - The hidden input element will get the required attribute, triggering native form validation on submit
* - The combobox input will have `aria-required` set to true
*
* @see {@link https://chakra-ui.com/docs/components/input/props}
* @see {@link https://chakra-ui.com/docs/components/form-control/props}
@@ -86,7 +87,7 @@ declare module "react-select/base" {
* @see {@link https://github.com/csandman/chakra-react-select#colorscheme}
* @see {@link https://chakra-ui.com/docs/components/tag/props}
*/
isRequired?: boolean;

Expand All @@ -86,7 +90,7 @@ declare module "react-select/base" {
* @see {@link https://github.com/csandman/chakra-react-select#colorscheme}
* @see {@link https://chakra-ui.com/docs/components/tag/props}
*/
colorScheme?: string;
colorScheme?: ColorScheme;

/**
* The `variant` prop that will be forwarded to your `MultiValue` component
Expand Down Expand Up @@ -132,12 +136,12 @@ declare module "react-select/base" {
* @defaultValue `blue`
* @see {@link https://github.com/csandman/chakra-react-select#selectedoptioncolorscheme--default-blue}
*/
selectedOptionColorScheme?: string;
selectedOptionColorScheme?: ColorScheme;

/**
* @deprecated Replaced by {@link selectedOptionColorScheme}
*/
selectedOptionColor?: string;
selectedOptionColor?: ColorScheme;

/**
* The color value to style the border of the `Control` with when the
Expand Down
12 changes: 5 additions & 7 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type {
Pseudos,
ResponsiveObject,
SystemStyleObject,
ThemeTypings,
} from "@chakra-ui/system";
import type {
ClearIndicatorProps,
Expand Down Expand Up @@ -45,16 +46,13 @@ export type Size = "sm" | "md" | "lg";

export type SizeProp = Size | ResponsiveObject<Size> | Size[];

export type TagVariant = "subtle" | "solid" | "outline" | (string & {});
export type TagVariant = ThemeTypings["components"]["Tag"]["variants"];

export type SelectedOptionStyle = "color" | "check";

export type Variant =
| "outline"
| "filled"
| "flushed"
| "unstyled"
| (string & {});
export type Variant = ThemeTypings["components"]["Input"]["variants"];

export type ColorScheme = ThemeTypings["colorSchemes"];
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


export type StylesFunction<ComponentProps> = (
provided: SystemStyleObject,
Expand Down
2 changes: 1 addition & 1 deletion src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type { Size, SizeProp } from "./types";
*
* Borrowed from the original `react-select` package
*
* @see {@link https://github.com/JedWatson/react-select/blob/edf5265ee0158c026c9e8527a6d0490a5ac2ef23/packages/react-select/src/utils.ts#L75-L110}
* @see {@link https://github.com/JedWatson/react-select/blob/2f94e8d/packages/react-select/src/utils.ts#L79-L110}
*/
export const cleanCommonProps = <
Option,
Expand Down
Loading