parkhojeong commented on code in PR #66714:
URL: https://github.com/apache/airflow/pull/66714#discussion_r3228640362
##########
airflow-core/src/airflow/ui/src/components/ui/Checkbox.tsx:
##########
@@ -25,13 +25,20 @@ export type CheckboxProps = {
readonly rootRef?: React.Ref<HTMLLabelElement>;
} & ChakraCheckbox.RootProps;
+const interactiveCursorProps = {
+ _disabled: { cursor: "not-allowed" },
+ cursor: "pointer",
+};
+
export const Checkbox = React.forwardRef<HTMLInputElement,
CheckboxProps>((props, ref) => {
const { children, icon, inputProps, rootRef, ...rest } = props;
return (
- <ChakraCheckbox.Root ref={rootRef} {...rest}>
+ <ChakraCheckbox.Root {...interactiveCursorProps} ref={rootRef} {...rest}>
Review Comment:
Thanks for the catch. Addressed In
- remove prop for Root:
https://github.com/apache/airflow/pull/66714/commits/e1abefba30b8b5b1d3c9b06d9f1ba3235be40696
- inline:
https://github.com/apache/airflow/pull/66714/commits/dddceb38d280fb7c9a01338727b5d3f73e0fddc8
---
I considered this for interactiveCursorProps vs inline
- `interactiveCursorProps` helps people who are unfamiliar with this
Chakra context, but it looks verbose.
- Inlining the props keeps things simpler, but it makes it less clear that
`_disabled` and `cursor` are related.
Inlining looks better here because this assumes familiarity with Chakra.
##########
airflow-core/src/airflow/ui/src/components/ui/Checkbox.tsx:
##########
@@ -25,13 +25,20 @@ export type CheckboxProps = {
readonly rootRef?: React.Ref<HTMLLabelElement>;
} & ChakraCheckbox.RootProps;
+const interactiveCursorProps = {
+ _disabled: { cursor: "not-allowed" },
+ cursor: "pointer",
+};
+
export const Checkbox = React.forwardRef<HTMLInputElement,
CheckboxProps>((props, ref) => {
const { children, icon, inputProps, rootRef, ...rest } = props;
return (
- <ChakraCheckbox.Root ref={rootRef} {...rest}>
+ <ChakraCheckbox.Root {...interactiveCursorProps} ref={rootRef} {...rest}>
Review Comment:
Thanks for the catch. Addressed
- remove prop for Root:
https://github.com/apache/airflow/pull/66714/commits/e1abefba30b8b5b1d3c9b06d9f1ba3235be40696
- inline:
https://github.com/apache/airflow/pull/66714/commits/dddceb38d280fb7c9a01338727b5d3f73e0fddc8
---
I considered this for interactiveCursorProps vs inline
- `interactiveCursorProps` helps people who are unfamiliar with this
Chakra context, but it looks verbose.
- Inlining the props keeps things simpler, but it makes it less clear that
`_disabled` and `cursor` are related.
Inlining looks better here because this assumes familiarity with Chakra.
##########
airflow-core/src/airflow/ui/src/components/ui/Checkbox.tsx:
##########
@@ -25,13 +25,20 @@ export type CheckboxProps = {
readonly rootRef?: React.Ref<HTMLLabelElement>;
} & ChakraCheckbox.RootProps;
+const interactiveCursorProps = {
+ _disabled: { cursor: "not-allowed" },
+ cursor: "pointer",
+};
+
export const Checkbox = React.forwardRef<HTMLInputElement,
CheckboxProps>((props, ref) => {
const { children, icon, inputProps, rootRef, ...rest } = props;
return (
- <ChakraCheckbox.Root ref={rootRef} {...rest}>
+ <ChakraCheckbox.Root {...interactiveCursorProps} ref={rootRef} {...rest}>
Review Comment:
Thanks for the catch. Addressed.
- remove prop for Root:
https://github.com/apache/airflow/pull/66714/commits/e1abefba30b8b5b1d3c9b06d9f1ba3235be40696
- inline:
https://github.com/apache/airflow/pull/66714/commits/dddceb38d280fb7c9a01338727b5d3f73e0fddc8
---
I considered this for interactiveCursorProps vs inline
- `interactiveCursorProps` helps people who are unfamiliar with this
Chakra context, but it looks verbose.
- Inlining the props keeps things simpler, but it makes it less clear that
`_disabled` and `cursor` are related.
Inlining looks better here because this assumes familiarity with Chakra.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
