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

Fix TextField and Callout TestSelectors #2303

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AnirudhMani-okta
Copy link
Contributor

OKTA-754200

Summary

Testing & Screenshots

  • I have confirmed this change with my designer and the Odyssey Design Team.

@@ -44,27 +44,14 @@ export const TextFieldTestSelectors = {
errorMessage: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unsure what to do with these features

Copy link
Contributor

Choose a reason for hiding this comment

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

  label: ["errorMessage", "hint", "label"],

That'll fix it.

Comment on lines 40 to 54
//labels ? something that is a child of the selector rather than a separate feature
// text: {
// selector: {
// method: "ByText",
// templateVariableNames: ["text"],
// text: "${text}",
// },
// },
// title: {
// selector: {
// method: "ByText",
// templateVariableNames: ["title"],
// text: "${title}",
// },
// },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//labels ? something that is a child of the selector rather than a separate feature
// text: {
// selector: {
// method: "ByText",
// templateVariableNames: ["text"],
// text: "${text}",
// },
// },
// title: {
// selector: {
// method: "ByText",
// templateVariableNames: ["title"],
// text: "${title}",
// },
// },
// label: ["description", "title"],

@@ -166,7 +166,7 @@ export const querySelector = <TestSelectors extends FeatureTestSelector>({

return {
element,
select,
selectChild,
Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta Jul 26, 2024

Choose a reason for hiding this comment

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

For anyone else looking at this PR, I asked for this renaming as we're gonna have a separate function for selecting labels.

},
}).element;

expect(element).toHaveErrorMessage(/This field is required/);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably have this quoted instead:

Suggested change
expect(element).toHaveErrorMessage(/This field is required/);
expect(element).toHaveErrorMessage("This field is required");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-08-14 at 3 57 27 PM

For some reason, it concatenates the "Error" part, so I can either regex match or add "Error"

Comment on lines +281 to +283
expect(element).toHaveErrorMessage(/This field is required:/);
expect(element).toHaveErrorMessage(/At least 2 chars/);
expect(element).toHaveErrorMessage(/No more than 20 chars/);
Copy link
Contributor

Choose a reason for hiding this comment

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

These can all be in quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to the other comment, toHaveErrorMessage concatenates everything into one string

Screenshot 2024-08-14 at 3 57 21 PM

@@ -246,6 +247,19 @@ export const Error: StoryObj<typeof TextField> = {
errorMessage: "This field is required.",
defaultValue: "",
},
play: async ({ canvasElement, step }) => {
await step("has visible label", async () => {
Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta Jul 26, 2024

Choose a reason for hiding this comment

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

Suggested change
await step("has visible label", async () => {
await step("has visible error message", async () => {

@@ -254,6 +268,21 @@ export const ErrorsList: StoryObj<typeof TextField> = {
errorMessageList: ["At least 2 chars", "No more than 20 chars"],
defaultValue: "",
},
play: async ({ canvasElement, step }) => {
await step("has visible label", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await step("has visible label", async () => {
await step("has visible error messages", async () => {

Comment on lines 309 to 311
expect(element).toHaveAccessibleDescription(
/Specify your destination within the Sol system./,
);
Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta Jul 26, 2024

Choose a reason for hiding this comment

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

Suggested change
expect(element).toHaveAccessibleDescription(
/Specify your destination within the Sol system./,
);
expect(element).toHaveAccessibleDescription(
"Specify your destination within the Sol system.",
);

I think the only places we can't use quotes are Callback, Banner, and Toast.

@KevinGhadyani-Okta
Copy link
Contributor

Updated Test Selectors code is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants