-
Notifications
You must be signed in to change notification settings - Fork 86
feat(text-input): add new next component #7557
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { default } from "./validation-message.component"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import React from "react"; | ||
| import StyledValidationMessage from "./validation-message.style"; | ||
| import tagComponent, { | ||
| TagProps, | ||
| } from "../../../__internal__/utils/helpers/tags"; | ||
|
|
||
| export interface ValidationMessageProps extends TagProps { | ||
| /** | ||
| * If true, displays error styling. If passed as a valid string, displays error styling | ||
| * and the message as children. | ||
| */ | ||
| error?: boolean | string; | ||
| /** Id of the component, to be used for accessibility purposes */ | ||
| id?: string; | ||
| /** | ||
| * If true, displays warning styling. If passed as a valid string, displays warning styling | ||
| * and the message as children. | ||
| */ | ||
| warning?: boolean | string; | ||
| /** If true, uses large font size */ | ||
| isLarge?: boolean; | ||
| } | ||
|
|
||
| const ValidationMessage = ({ | ||
| error, | ||
| id, | ||
| warning, | ||
| "data-element": dataElement, | ||
| "data-role": dataRole = "validation-message", | ||
| isLarge, | ||
| }: ValidationMessageProps) => { | ||
| const validation = error || warning; | ||
| const isStringValidation = typeof validation === "string"; | ||
|
|
||
| return isStringValidation ? ( | ||
| <StyledValidationMessage | ||
| id={id} | ||
| isError={Boolean(error)} | ||
| {...tagComponent("validation-message", { | ||
| "data-element": dataElement, | ||
| "data-role": dataRole, | ||
| })} | ||
| isLarge={isLarge} | ||
| > | ||
| {validation} | ||
| </StyledValidationMessage> | ||
| ) : null; | ||
| }; | ||
|
|
||
| export default ValidationMessage; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import styled, { css } from "styled-components"; | ||
|
|
||
| interface StyledValidationMessageProps { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (non-blocking): these won;'t be added to the DOM element so up to you |
||
| isError?: boolean; | ||
| isDarkBackground?: boolean; | ||
| isLarge?: boolean; | ||
| } | ||
|
|
||
| const StyledValidationMessage = styled.span<StyledValidationMessageProps>` | ||
| ${({ isError, isLarge }) => { | ||
| return css` | ||
| display: flex; | ||
| align-items: center; | ||
| align-content: center; | ||
| align-self: stretch; | ||
| flex-wrap: wrap; | ||
| margin: 0px; | ||
| margin: 0px; | ||
| color: ${isError | ||
| ? "var(--input-validation-label-error)" | ||
| : "var(--input-validation-label-warn)"}; | ||
| font-family: var(--fontFamiliesDefault); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: why aren't we using the new tokens for the font?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment: The font tokens contain everything already as far as I know so you could just set can be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @edleeks87 i don't believe there's any nice way to show hint text inline. When switching it on within figma it looks identical to Tom's implementation. TBF inline is generally discouraged. Where is see it used, it never has hint text enabled. e.g for quick filtering on top of tables. I think we have 2 options:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think personally we should keep inline input hints, if we're providing the capacity to make labels inline via |
||
| font-size: ${isLarge | ||
| ? "var(--global-font-static-body-regular-l)" | ||
| : "var(--global-font-static-body-regular-m)"}; | ||
| font-style: normal; | ||
| font-weight: ${isError ? "500" : "400"}; | ||
| line-height: 150%; | ||
|
Comment on lines
+32
to
+33
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: would these need to be tokens, too? Or are we leaving those aside for now?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@damienrobson-sage we don't currently distribute core tokens like line-height or font-weight. instead Font tokens can be applied which contain those core values.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, this is due to a little misunderstanding on my part, some hard coded font-familys, weights, colours, sizes etc. have been used instead of the font shorthand with the corresponding font token. I'll make sure this is addressed where i've used them 👍 |
||
| `; | ||
| }} | ||
| `; | ||
|
|
||
| export default StyledValidationMessage; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| import React from "react"; | ||
| import { render, screen } from "@testing-library/react"; | ||
| import ValidationMessage from "."; | ||
|
|
||
| test("should not render when neither error nor warning is provided", () => { | ||
| render(<ValidationMessage data-role="validation-message" />); | ||
| expect(screen.queryByTestId("validation-message")).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test("should render when error is a string", () => { | ||
| render(<ValidationMessage error="Error message" />); | ||
| const validationMessage = screen.getByText("Error message"); | ||
| expect(validationMessage).toBeVisible(); | ||
| }); | ||
|
|
||
| test("should render when warning is a string", () => { | ||
| render(<ValidationMessage warning="Warning message" />); | ||
| const validationMessage = screen.getByText("Warning message"); | ||
| expect(validationMessage).toBeVisible(); | ||
| }); | ||
|
|
||
| test("should render with id attribute", () => { | ||
| render(<ValidationMessage id="validation-error-1" error="Error message" />); | ||
| const validationMessage = screen.getByText("Error message"); | ||
| expect(validationMessage).toHaveAttribute("id", "validation-error-1"); | ||
| }); | ||
|
|
||
| test("should support custom data-role attribute", () => { | ||
| render( | ||
| <ValidationMessage error="Error occurred" data-role="custom-validation" />, | ||
| ); | ||
| const validationMessage = screen.getByTestId("custom-validation"); | ||
| expect(validationMessage).toBeVisible(); | ||
| }); | ||
|
|
||
| test("should support data-element attribute", () => { | ||
| render( | ||
| <ValidationMessage error="Error message" data-element="validation-error" />, | ||
| ); | ||
| const validationMessage = screen.getByText("Error message"); | ||
| expect(validationMessage).toHaveAttribute("data-element", "validation-error"); | ||
| }); | ||
|
|
||
| test("should apply error color when error is provided", () => { | ||
| render(<ValidationMessage error="Error message" />); | ||
| const validationMessage = screen.getByText("Error message"); | ||
|
|
||
| expect(validationMessage).toHaveStyle( | ||
| "color: var(--input-validation-label-error)", | ||
| ); | ||
| }); | ||
|
|
||
| test("should apply error font-weight", () => { | ||
| render(<ValidationMessage error="Error message" />); | ||
| const validationMessage = screen.getByText("Error message"); | ||
| expect(validationMessage).toHaveStyle("font-weight: 500"); | ||
| }); | ||
|
|
||
| test("should apply warning color when warning is provided", () => { | ||
| render(<ValidationMessage warning="Warning message" />); | ||
| const validationMessage = screen.getByText("Warning message"); | ||
| expect(validationMessage).toHaveStyle( | ||
| "color: var(--input-validation-label-warn)", | ||
| ); | ||
| }); | ||
|
|
||
| test("should apply warning font-weight", () => { | ||
| render(<ValidationMessage warning="Warning message" />); | ||
| const validationMessage = screen.getByText("Warning message"); | ||
| expect(validationMessage).toHaveStyle("font-weight: 400"); | ||
| }); | ||
|
|
||
| test("should apply medium font size by default for error", () => { | ||
| render(<ValidationMessage error="Error message" />); | ||
| const validationMessage = screen.getByText("Error message"); | ||
| expect(validationMessage).toHaveStyle( | ||
| "font-size: var(--global-font-static-body-regular-m)", | ||
| ); | ||
| }); | ||
|
|
||
| test("should apply large font size when isLarge is true for error", () => { | ||
| render(<ValidationMessage error="Error message" isLarge={true} />); | ||
| const validationMessage = screen.getByText("Error message"); | ||
| expect(validationMessage).toHaveStyle( | ||
| "font-size: var(--global-font-static-body-regular-l)", | ||
| ); | ||
| }); | ||
|
|
||
| test("should apply medium font size by default for warning", () => { | ||
| render(<ValidationMessage warning="Warning message" />); | ||
| const validationMessage = screen.getByText("Warning message"); | ||
| expect(validationMessage).toHaveStyle( | ||
| "font-size: var(--global-font-static-body-regular-m)", | ||
| ); | ||
| }); | ||
|
|
||
| test("should apply large font size when isLarge is true for warning", () => { | ||
| render(<ValidationMessage warning="Warning message" isLarge={true} />); | ||
| const validationMessage = screen.getByText("Warning message"); | ||
| expect(validationMessage).toHaveStyle( | ||
| "font-size: var(--global-font-static-body-regular-l)", | ||
| ); | ||
| }); | ||
|
|
||
| test("should prioritize error over warning when both provided", () => { | ||
| render(<ValidationMessage error="Error message" warning="Warning message" />); | ||
| expect(screen.getByText("Error message")).toBeVisible(); | ||
| expect(screen.queryByText("Warning message")).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test("should apply error styling when both error and warning strings are provided", () => { | ||
| render(<ValidationMessage error="Error message" warning="Warning message" />); | ||
| const validationMessage = screen.getByText("Error message"); | ||
| expect(validationMessage).toHaveStyle( | ||
| "color: var(--input-validation-label-error)", | ||
| ); | ||
| expect(validationMessage).toHaveStyle("font-weight: 500"); | ||
| }); |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment (non-blocking): if this is the same for all components I wonder if it's more appropriate in the shared |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import styled, { css } from "styled-components"; | ||
|
|
||
| const ErrorBorder = styled.span` | ||
| ${({ warning }: { warning: boolean }) => css` | ||
| width: 2px; | ||
tomdavies73 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| position: absolute; | ||
| left: -10px; | ||
| top: 0px; | ||
tomdavies73 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| bottom: 0px; | ||
| z-index: 6; | ||
| background-color: ${warning | ||
| ? "var(--input-validation-bar-warn)" | ||
| : "var(--input-validation-border-error)"}; | ||
| `} | ||
| transform: scaleX(1); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: is this transform doing anything here as it's scaling to 100% with no animation?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I added this as when the window is being repositioned the absolutely positioned error border can sometimes shrink when the label is inline. We didn't observe this previously as the error border was always in a fixed position, however when inline it is in the centre of the screen and needs to reposition. Adding the transform stopped this shrinking from occurring, and it appeared to be the only solution I could find that worked. |
||
| `; | ||
|
|
||
| export default ErrorBorder; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import React from "react"; | ||
| import { render, screen } from "@testing-library/react"; | ||
| import ErrorBorder from "./error-border.style"; | ||
|
|
||
| test("when `warning` is true should render with warning background colour", () => { | ||
| render(<ErrorBorder data-role="error-border" warning />); | ||
|
|
||
| expect(screen.getByTestId("error-border")).toHaveStyleRule( | ||
| "background-color", | ||
| "var(--input-validation-bar-warn)", | ||
| ); | ||
| }); | ||
|
|
||
| test("when `warning` is false should render with error background colour", () => { | ||
| render(<ErrorBorder data-role="error-border" warning={false} />); | ||
|
|
||
| expect(screen.getByTestId("error-border")).toHaveStyleRule( | ||
| "background-color", | ||
| "var(--input-validation-border-error)", | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import React from "react"; | ||
| import StyledHintText from "./hint-text.style"; | ||
|
|
||
| export interface HintTextProps { | ||
| /** Children elements */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: |
||
| children?: React.ReactNode; | ||
| /** Sets the id for the component */ | ||
| id?: string; | ||
| /** If true, uses large font size */ | ||
| isLarge?: boolean; | ||
| /** If true, the hint text will display in disabled styling */ | ||
| disabled?: boolean; | ||
| } | ||
|
|
||
| export const HintText = ({ | ||
| children, | ||
| id, | ||
| isLarge, | ||
| disabled, | ||
| }: HintTextProps) => { | ||
| return ( | ||
| <StyledHintText | ||
| data-element="input-hint" | ||
| data-role="hint-text" | ||
| id={id} | ||
| isLarge={isLarge} | ||
| disabled={disabled} | ||
| > | ||
| {children} | ||
| </StyledHintText> | ||
| ); | ||
| }; | ||
|
|
||
| export default HintText; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import styled from "styled-components"; | ||
|
|
||
| interface StyledHintTextProps { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: |
||
| isLarge?: boolean; | ||
| disabled?: boolean; | ||
| } | ||
|
|
||
| const StyledHintText = styled.span<StyledHintTextProps>` | ||
| color: ${({ disabled }) => | ||
| disabled | ||
| ? "var(--input-labelset-label-disabled)" | ||
| : "var(--input-labelset-label-alt)"}; | ||
| font-family: var(--fontFamiliesDefault); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment: same as above wrt to using |
||
| font-size: ${({ isLarge }) => | ||
| isLarge | ||
| ? "var(--global-font-static-body-regular-l)" | ||
| : "var(--global-font-static-body-regular-m)"}; | ||
| font-style: normal; | ||
| font-weight: 400; | ||
| line-height: 150%; | ||
| `; | ||
|
|
||
| export default StyledHintText; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: we need to consider what we need to still capture for the old textbox visual regression wise, again worth discussing with the rest of the team in slack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - maybe use one
TextInputand oneTextboxto cover off both?