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

Jump to bottom

[Listbox] Hide input that documentation claims is hidden #570

Merged
merged 3 commits into from May 11, 2020
Merged

[Listbox] Hide input that documentation claims is hidden #570

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1125136
listbox: hide input (#569)
Apr 30, 2020
6e2c2b6
listbox: simplify unit tests
May 1, 2020
6e150dd
listbox: simplify another unit test
May 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion packages/listbox/__tests__/listbox.test.tsx
View file
Open in desktop
Expand Up @@ -197,6 +197,10 @@ describe("<Listbox />", () => {
</Listbox>
);
expect(container.querySelector("input")).toBeTruthy();
expect(container.querySelector("input")).toHaveAttribute(
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead test that the input is visibly hidden with expect(container.querySelector("input")).not.toBeVisible()? I'll probably change the implementation of this in the near future to address arrow form navigation on iOS.

"type",
"hidden"
);
});
});

Expand All @@ -218,7 +222,12 @@ describe("<Listbox />", () => {
</Listbox>
</div>
);
expect(container.querySelector("input")).toBeTruthy();
// Be careful not to match the `input[type=text]` added in this test
expect(container.querySelector("input[type=hidden]")).toBeTruthy();
expect(container.querySelector("input[type=hidden]")).toHaveAttribute(
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove these assertions, see comment above about future implementation 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.

Sure. Do you mean restoring the test to how it was? Or just removing all assertions in this test?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can revert these lines to the one line from before. Let's just keep the check for a standard input and use the not.toBeVisible method to make sure it's hidden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 2 inputs, the first added by the test (<input type="text" name="name" /> not hidden) and the other added by listbox (which is hidden).

So the test you propose fails.

Do you like this option which will work to get that second input?

 expect(container.querySelectorAll("input")[1]).not.toBeVisible();

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 went ahead and pushed that up along with fixing the other line.

"form",
"my-form"
);
});
});

Expand All @@ -232,6 +241,10 @@ describe("<Listbox />", () => {
</Listbox>
);
expect(container.querySelector("input")).toBeTruthy();
expect(container.querySelector("input")).toHaveAttribute(
"type",
"hidden"
);
expect(container.querySelector("input")).toHaveAttribute("required");
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/listbox/src/index.tsx
View file
Open in desktop
Expand Up @@ -331,7 +331,7 @@ export const ListboxInput = forwardRef<
readOnly
required={required}
tabIndex={-1}
type="text"
type="hidden"
value={state.context.value || ""}
/>
)}
Expand Down