RadioGroup #25
RadioGroup #25
Conversation
…omponent-library into update-packaging
| </label> | ||
| {errMessage ? ( | ||
| <div | ||
| className={styles.errorMessage} |
There was a problem hiding this comment.
QUERY: Can we detext if there is a link in the label to add a warning?
JackWP1
left a comment
JackWP1
left a comment
There was a problem hiding this comment.
Okay I'm done for now. The library is looking great. 👍 Only minor comments or questions
| @@ -0,0 +1,16 @@ | |||
| import React, { forwardRef } from "react"; | |||
There was a problem hiding this comment.
QUERY: Do we still need the React import here?
| @@ -0,0 +1,16 @@ | |||
| import React from "react"; | |||
| import { describe, it, expect, afterEach } from "vitest"; | |||
| import { render, cleanup, screen } from "@testing-library/react"; | |||
There was a problem hiding this comment.
SAND: screen is not being used here but probably useful to leave in
There was a problem hiding this comment.
yeah, that was the thought
| with: | ||
| node-version: "16" | ||
|
|
||
| - name: Install dependencies |
There was a problem hiding this comment.
QUERY: Should there be a query to check the branch is up-to-date with master?
There was a problem hiding this comment.
This is a start, Ben is doing the rest
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
QUERY: I thought this file type was in the gitignore?
There was a problem hiding this comment.
I removed the gitignore because I wanted to suggest some extensions
| <AerAccordion.Root {...args}> | ||
| <AerAccordion.Item value="item-1"> | ||
| <AerAccordion.Header>Header 1</AerAccordion.Header> | ||
| <AerAccordion.Content> |
There was a problem hiding this comment.
SAND: It would be nice to show a default expanded state on one of these.
| /** A button to trigger the alert dialog to open */ | ||
| trigger: ReactElement<HTMLButtonElement>; | ||
| /** The title and whether to hide it (this defaults to `false`). NOTE: This is placed in an `<h2>`. */ | ||
| dialogTitle: string | ReactElement<unknown> | HideableTextShape; |
There was a problem hiding this comment.
QUERY: Why is this required if it can be hidden an it's not in the HideableTextShape? I'm unsure what elementIsHideableTextShape is doing?
There was a problem hiding this comment.
This is required because the title is required to make the alert accessible. Nevertheless you may visually hide the title. The is elementIsHideableTextShape is just a type gaurd to allow easy checks
| </AerAlertDialog.Root>, | ||
| ); | ||
|
|
||
| await userEvent.click( |
There was a problem hiding this comment.
SAND: I don't think this await is doing anything here
There was a problem hiding this comment.
It is. The latest version of userEvent is all async
| @@ -0,0 +1,125 @@ | |||
| // Vitest Snapshot v1 | |||
| expect(screen.queryByRole("status")).toBeFalsy(); | ||
| expect(onBlurMock).not.toHaveBeenCalled(); | ||
|
|
||
| await userEvent.click(checkbox); |
There was a problem hiding this comment.
SAND: This and other awaits can be removed in these tests
| export interface AerCheckboxProps | ||
| extends Omit<DefaultProps<"input">, "checked"> { | ||
| // Required label for the checkbox. NOTE: you can hide the label using the `LabelProps` API, but must provide a label to make the field accessible | ||
| label: string | ReactElement | LabelProps; |
There was a problem hiding this comment.
QUERY: I feel that rather than relying on the dev to pass a correctly implemented label, that we define it within the component so it will always be there..?
No description provided.