Checkbox #23
Checkbox #23
Conversation
S-unya
commented
Nov 28, 2022
S-unya
commented
- Create new AerCheckbox component
- Update some patterns in the other components
- Update templates
- Create docs/ remove docs
- Removed some unused files
…omponent-library into update-packaging
S-unya
requested review from
C0deJack,
DanCoderDev,
g-pex-aerian,
hannahwilliams,
olibooty and
waynepitts
There was a problem hiding this comment.
QUERY: Are we planning on adding ESLint/prettier to this repo?
SAND: export { applyTheme } from "./Theme"; needs removing from src/index.tsx
It might be nice (if we have time) make a small example repo that sits alongside this project.
No show stoppers. 👍 PRs are always a negative process by default, picking holes in other peoples work. Great to see a lot of gotchas have been dealt with already. Great start. Thanks S'unya!
| @@ -0,0 +1,90 @@ | |||
| import React, { ForwardedRef, forwardRef, ReactElement } from "react"; | |||
There was a problem hiding this comment.
QUERY: Are we calling our component Dialog rather than Dialogue ?
There was a problem hiding this comment.
src/main.tsx
Outdated
| import { | ||
| AerAlertDialog, | ||
| AerAlertDialogFooter, | ||
| AerAlertDialogTrigger, |
|
|
||
| // Official docs suggest the non-null assertion | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const root = createRoot(container!); |
There was a problem hiding this comment.
SAND: I guess you don't need the disable rule and the bang?
There was a problem hiding this comment.
The disable rule is because we are adding the !, which asserts that the element exists (even though it is possibly undefined)
| throw new Error( | ||
| "use{{pascalCase name}} must be used within an {{pascalCase name}}Provider" | ||
| ); | ||
| } |
There was a problem hiding this comment.
PEBBLE: Formatting is a bit off here, and above.
There was a problem hiding this comment.
Yes, there is no good formatter for these templates. Once the code has generated and you hit save, it will all fix itself
| @@ -0,0 +1,341 @@ | |||
| /*! | |||
| @@ -1,14 +1,15 @@ | |||
| import React, { HTMLAttributes } from "react"; | |||
| import * as yup from "yup"; | |||
| import { useForm, SubmitHandler } from "react-hook-form"; | |||
| import { yupResolver } from "@hookform/resolvers"; | |||
| import { yupResolver } from "@hookform/resolvers/yup"; | |||
| import cx from "classnames"; | |||
|
|
|||
| import { FormContext } from "./useFormContext"; | |||
|
|
|||
| import styles from "./Form.module.scss"; | |||
| import { UnpackNestedValue } from "react-hook-form/dist/types/form"; | |||
| const [defaultValues, setDefaultValues] = React.useState< | ||
| UnpackNestedValue<DeepPartial<T>> | ||
| >(); | ||
| const [defaultValues, setDefaultValues] = React.useState<T>(); | ||
|
|
||
| const methods = useForm<T>({ | ||
| resolver: yupResolver(wrappedValidationSchema), |
|
|
||
| .indicator { | ||
| position: relative; | ||
| color: var(--c-cmp-checbox-icon, currentColor); |
| font-size: var(--t-cmp-checkbox, var(--c-body-m)); | ||
| } | ||
|
|
||
| .invalid { |
There was a problem hiding this comment.
QUERY: could this be combined with .errorMessage ?
There was a problem hiding this comment.
They may want to be different colours, they just happen to be the same at the moment
| const resolveNextState = ( | ||
| currentState: CheckedStates, | ||
| checked: CheckedStates | ||
| ) => (checked === "indeterminate" ? "indeterminate" : !currentState); |
There was a problem hiding this comment.
QUERY: Does the state of indeterminate cover a loading state for this component? If not, is it worth exploring adding a loading state?
There was a problem hiding this comment.
:shrugging: I'm not sure. I can't really see why a checkbox should be in a loading state... but let's call YAGNI on it and add it if it is ever needed.
