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.

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

Various small code fixes / type fixes #3313

Merged
merged 3 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions src/DataTable/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ To enable proper selection behavior with backend pagination (i.e., when ``isSele
</Component>
);

const ClearAction = ({ as: Component, tableInstance, ...rest }) => (
const ClearAction = ({ as: Component, tableInstance }) => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rest parameter was unused.

Fixes: 'rest' is declared but its value is never read.

<Component
variant="danger"
onClick={() => {
Expand Down Expand Up @@ -845,7 +845,7 @@ a responsive grid of cards.
</Component>
);

const ClearAction = ({ as: Component, tableInstance, ...rest }) => (
const ClearAction = ({ as: Component, tableInstance }) => (
<Component
variant="danger"
onClick={() => {
Expand Down
2 changes: 1 addition & 1 deletion src/Dropdown/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ You can use `Dropdown.Toggle` with [IconButton](/components/iconbutton) componen
```jsx live
<Dropdown.Deprecated>
<Dropdown.Deprecated.Button>
<Icon className="fa fa-user pr-3" alt="" />
<Icon className="fa fa-user pr-3" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

<Icon> does not accept an alt prop. It accepts an optional screenReaderText but it can be omitted for decorative icons, to emulate alt=""

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

Search Engines
</Dropdown.Deprecated.Button>
<Dropdown.Deprecated.Menu>
Expand Down
2 changes: 1 addition & 1 deletion src/Dropzone/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ This example validates that only `400x479` images can be uploaded.
async function imageDimensionValidator(file) {
const image = new window.Image();
try {
url = URL.createObjectURL(file);
const url = URL.createObjectURL(file);
image.src = url;
await image.decode();
if (image.width !== 400 || image.height !== 479) {
Expand Down
10 changes: 4 additions & 6 deletions src/Form/form-control.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ or [select attributes](https://developer.mozilla.org/en-US/docs/Web/HTML/Element
() => {
{/* start example state */}
const [type, setType] = useState('default');
const [value, setValue] = useState('');
{/* end example state */}
const handleChange = (e) => setValue(e.target.value);

const inputs = {
default: (
Expand Down Expand Up @@ -144,8 +146,6 @@ or [select attributes](https://developer.mozilla.org/en-US/docs/Web/HTML/Element
</Form.Group>
</>),
};
const [value, setValue] = useState('');
const handleChange = (e) => setValue(e.target.value);
return (
<>
{/* start example form block */}
Expand All @@ -172,7 +172,9 @@ See [react-imask](https://imask.js.org) for documentation on available props.
() => {
{/* start example state */}
const [maskType, setMaskType] = useState('phone');
const [value, setValue] = useState('');
{/* end example state */}
const handleChange = (e) => setValue(e.target.value);

const inputsWithMask = {
phone: (
Expand Down Expand Up @@ -255,10 +257,6 @@ See [react-imask](https://imask.js.org) for documentation on available props.
),
};

const [value, setValue] = useState('');

const handleChange = (e) => setValue(e.target.value);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes: Block-scoped variable 'value' used before its declaration.

return (
<>
{/* start example form block */}
Expand Down
5 changes: 5 additions & 0 deletions src/IconButton/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ interface Props extends React.HTMLAttributes<HTMLButtonElement> {
variant?: 'primary' | 'secondary' | 'success' | 'warning' | 'danger' | 'light' | 'dark' | 'black' | 'brand';
/** size of button to render */
size?: 'sm' | 'md' | 'inline';
/** Used with `IconButtonToggle` */
value?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see in the docs, <IconButtonToggle> requires its <IconButton> children to accept a value prop, but that wasn't defined in the prop types.

Fixed a type error:

Property 'value' does not exist on type 'IntrinsicAttributes & Pick<Pick<PropsWithTooltip, "variant" | "className" | "children" | "slot" | "style" | "title" | "size" | "defaultChecked" | ... 260 more ... | "tooltipContent"> & Pick<...> & Pick<...>, "tooltipContent"> & Partial<...> & Partial<...>'.

/** no children */
children?: never;
}
Expand Down Expand Up @@ -104,6 +106,7 @@ IconButton.defaultProps = {
size: 'md',
onClick: () => {},
isActive: false,
value: undefined,
children: undefined,
};

Expand Down Expand Up @@ -140,6 +143,8 @@ IconButton.propTypes = {
size: PropTypes.oneOf(['sm', 'md', 'inline']),
/** whether to show the `IconButton` in an active state, whose styling is distinct from default state */
isActive: PropTypes.bool,
/** Used with <IconButtonToggle> */
value: PropTypes.string,
};

interface PropsWithTooltip extends Props {
Expand Down
5 changes: 2 additions & 3 deletions src/InputSelect/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,13 @@ notes: |
label="Favorite Color"
options={['', 'red', 'orange', 'yellow', 'green', 'blue', 'purple']}
validator={value => {
let feedback = { isValid: true };
if (!value) {
feedback = {
return {
isValid: false,
validationMessage: 'Please make a selection.',
};
}
return feedback;
return { isValid: true };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change (A) simplifies the validator function, making it a bit more clear, and (B) helps TypeScript avoid a type inference mistake: Object literal may only specify known properties, and 'validationMessage' does not exist in type '{ isValid: boolean; }'.

}}
/>
```
Expand Down
10 changes: 4 additions & 6 deletions src/InputText/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,13 @@ notes: |
label="Username"
description="The unique name that identifies you throughout the site."
validator={value => {
let feedback = { isValid: true };
if (value.length < 3) {
feedback = {
return {
isValid: false,
validationMessage: 'Username must be at least 3 characters in length.',
};
}
return feedback;
return { isValid: true };
}}
/>
```
Expand All @@ -53,15 +52,14 @@ notes: |
label="Username"
description="The unique name that identifies you throughout the site."
validator={value => {
let feedback = { isValid: true };
if (value.length < 3) {
feedback = {
return {
isValid: false,
validationMessage: 'Username must be at least 3 characters in length.',
dangerIconDescription: 'Error',
};
}
return feedback;
return { isValid: true };
}}
themes={['danger']}
/>
Expand Down
2 changes: 1 addition & 1 deletion src/ListBox/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class ListBoxWrapperForOnSelect extends React.Component {
<span className="sr-only">none</span>
) : (
<span
arialabelledby={`list-box-option-${
aria-labelledby={`list-box-option-${
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Property 'arialabelledby' does not exist on type 'DetailedHTMLProps<HTMLAttributes<HTMLSpanElement>, HTMLSpanElement>'. Did you mean '"aria-labelledby"'?

this.state.selectedOptionIndex
}`}
>
Expand Down
2 changes: 1 addition & 1 deletion src/Modal/standard-modal.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ The standard `ModalDialog` composition. `StandardModal` passes all of its props
footerNode={(
<ActionRow>
<p className="small">
<Hyperlink href="#">Get help</Hyperlink>
<Hyperlink destination="#">Get help</Hyperlink>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although this was working, the <Hyperlink> component expects destination as a prop, not href.

</p>
<ActionRow.Spacer />
<Button variant="tertiary" onClick={close}>Cancel</Button>
Expand Down
5 changes: 2 additions & 3 deletions src/TextArea/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,13 @@ notes: |
label="Username"
description="The unique name that identifies you throughout the site."
validator={value => {
let feedback = { isValid: true };
if (value.length < 3) {
feedback = {
return {
isValid: false,
validationMessage: 'Username must be at least 3 characters in length.',
};
}
return feedback;
return { isValid: true };
}}
/>
```
Expand Down
2 changes: 1 addition & 1 deletion src/utils/propTypes/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const isEveryPropDefined = (props, otherPropNames) => otherPropNames
* Returns a PropType entry with the given propType that is required if otherPropName
* is truthy.
* @param {func} propType - target PropType
* @param {string} otherPropName - string name for prop that, if true, marks the
* @param {string | string[]} otherPropName - string name for prop that, if true, marks the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see in the implementation below as well as how this function is used, this param accepts a string array.

* associated prop as required
* @return {func} - PropType based on propType that is required if otherPropName is
* set to true.
Expand Down
31 changes: 10 additions & 21 deletions www/src/components/exampleComponents/HipsterIpsum.tsx
bradenmacdonald marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React from 'react';
import PropTypes from 'prop-types';

/**
* Randomly shuffle an array
Expand Down Expand Up @@ -48,18 +47,17 @@ const shortParagraphs = [
'Pinterest tote bag synth, tattooed echo park cronut flannel kombucha kickstarter viral.',
];

export type HipsterIpsumType = {
numParagraphs: number,
numShortParagraphs: number,
};
export type HipsterIpsumType = { numParagraphs?: number; numShortParagraphs?: number; };

const getHipsterIpsumContent = (allParagraphs: Array<string>, numParagraphs: number) => allParagraphs
.slice(0, numParagraphs)
.map(text => <p key={text}>{text}</p>);
const getHipsterIpsumContent = (allParagraphs: Array<string>, numParagraphs: number) => (
<>
{allParagraphs.slice(0, numParagraphs).map(text => <p key={text}>{text}</p>)}
</>
);

export type HipsterIpsumContentType = {
shortParagraphsInContent: string[],
numShortParagraphs: number,
numShortParagraphs?: number,
paragraphsInContent: string[],
numParagraphs: number,
};
Expand All @@ -79,26 +77,17 @@ const useHipsterIpsumContent = ({
return getHipsterIpsumContent(shuffledParagraphs, numParagraphs);
};

function HipsterIpsum({
numParagraphs,
const HipsterIpsum: React.FC<HipsterIpsumType> = ({
numParagraphs = 2,
numShortParagraphs,
}: HipsterIpsumType) {
}) => {
const content = useHipsterIpsumContent({
shortParagraphsInContent: shortParagraphs,
numShortParagraphs,
paragraphsInContent: paragraphs,
numParagraphs,
});
return content;
}

HipsterIpsum.propTypes = {
numParagraphs: PropTypes.number,
numShortParagraphs: PropTypes.number,
};
HipsterIpsum.defaultProps = {
numParagraphs: 2,
numShortParagraphs: undefined,
};

export default HipsterIpsum;
Loading