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

feat: allow custom help text class #761

Merged
merged 1 commit into from
Apr 20, 2022
Merged

feat: allow custom help text class #761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 11 additions & 2 deletions src/components/Field/Field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ export type Props = {
* Help text to show below the field.
*/
help?: ReactNode;
/**
* Optional class(es) to pass to the help text element.
*/
helpClassName?: ReactNode;
Copy link
Member

Choose a reason for hiding this comment

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

I guess would be good to have some convention for adding those child class names, I think some other components do it as well (don't remember which one).

Not sure how detailed or strict we want to be on which elements have configurable class name and which don't. But at least some prop naming convention would be good.

elementClassName seems fine for start (and consistency with default className.

Copy link
Contributor Author

@petermakowski petermakowski Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed that this is needed (and I like your suggested naming convention).

I think we should have a common way of translating base/modifier classes that exist in vanilla to props in react-components as well, e.g. the help prop in Input should probably be named formHelpText instead to match p-form-help-text from Vanilla. There are many examples for inconsistencies like that.

This would make working with react-components more intuitive for people who are already familiar with the vanilla framework (and vice-versa).

If we agree on a naming convention it might be worth updating all components props at some point even though there will be breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created an issue for this #762

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering that the related element in vanilla has a base class p-form-help-text, would you consider a name of the element to be formHelpText? That would result in formHelpTextClassName instead of helpClassName.

A bit lengthy, but I don't think that matters - having prop names that are long but consistent is much better than having props that are shorter but different in vanilla and react-components.

Copy link
Member

@bartaz bartaz Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately form components in Vanilla have very inconsistent and lengthy names. There is no "p-form", but we do have "p-form-others"… and there are more issues like that, so forms are definitely not something we should base the naming convention on 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, should we go with my initial suggestion (helpClassName) then? It's consistent with the related help prop that already exists on this component.

/**
* An id to give to the help element.
*/
Expand Down Expand Up @@ -80,11 +84,12 @@ export type Props = {
const generateHelpText = ({
help,
helpId,
helpClassName,
isTickElement,
}: Pick<Props, "help" | "helpId" | "isTickElement">) =>
}: Pick<Props, "help" | "helpId" | "helpClassName" | "isTickElement">) =>
help ? (
<p
className={classNames("p-form-help-text", {
className={classNames("p-form-help-text", helpClassName, {
"is-tick-element": isTickElement,
})}
id={helpId}
Expand Down Expand Up @@ -138,6 +143,7 @@ const generateContent = ({
labelFirst,
labelNode,
help,
helpClassName,
error,
caution,
success,
Expand All @@ -159,6 +165,7 @@ const generateContent = ({
{generateHelpText({
helpId,
help,
helpClassName,
isTickElement,
})}
{generateError(error, caution, success, validationId)}
Expand All @@ -172,6 +179,7 @@ const Field = ({
error,
forId,
help,
helpClassName,
helpId,
isSelect,
isTickElement,
Expand All @@ -198,6 +206,7 @@ const Field = ({
labelFirst,
labelNode,
help,
helpClassName,
error,
caution,
success,
Expand Down
6 changes: 6 additions & 0 deletions src/components/Input/Input.stories.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ import Input from "./Input";
type: "text",
},
},
helpClassName: {
control: {
type: "text",
},
},
label: {
control: {
type: "text",
Expand Down Expand Up @@ -67,6 +72,7 @@ An input field where the user can enter data, which can vary in many ways, depen
placeholder: "[email protected]",
label: "Email address",
help: "Additional description for the field",
helpClassName: "u-no-margin--bottom",
}}
>
{Template.bind({})}
Expand Down
26 changes: 19 additions & 7 deletions src/components/Input/Input.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,6 @@ import React from "react";
import Input from "./Input";

describe("Input", () => {
it("can add additional classes", () => {
const wrapper = shallow(<Input type="text" className="extra-class" />);
const className = wrapper.find("input").prop("className");
expect(className.includes("p-form-validation__input")).toBe(true);
expect(className.includes("extra-class")).toBe(true);
});

it("moves the label for radio buttons", () => {
const wrapper = shallow(<Input type="radio" />);
expect(wrapper.prop("label")).toBe("");
Expand Down Expand Up @@ -112,4 +105,23 @@ describe("Input RTL", () => {
render(<Input help={help} type="checkbox" />);
expect(screen.getByRole("checkbox")).toHaveAccessibleDescription(help);
});

it("can add additional classes", () => {
const { container } = render(
<Input
type="text"
className="extra-class"
help="additional description"
helpClassName="additional-help-text-class"
/>
);
expect(screen.getByRole("textbox")).toHaveClass(
"p-form-validation__input",
"extra-class"
);
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
expect(container.querySelector(".p-form-help-text")).toHaveClass(
"additional-help-text-class"
);
});
});
6 changes: 6 additions & 0 deletions src/components/Input/Input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ export type Props = PropsWithSpread<
* Help text to show below the field.
*/
help?: ReactNode;
/**
* Optional class(es) to pass to the help text element.
*/
helpClassName?: ReactNode;
/**
* The id of the input.
*/
Expand Down Expand Up @@ -71,6 +75,7 @@ const Input = ({
className,
error,
help,
helpClassName,
id,
label,
labelClassName,
Expand Down Expand Up @@ -138,6 +143,7 @@ const Input = ({
error={error}
forId={id}
help={help}
helpClassName={helpClassName}
helpId={helpId}
isTickElement={type === "checkbox" || type === "radio"}
label={fieldLabel}
Expand Down