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

chore(BaseInput): reusable, internal Input component #2053

Merged
merged 13 commits into from
Apr 17, 2024

Conversation

YossiSaadi
Copy link
Contributor

@YossiSaadi YossiSaadi commented Apr 9, 2024

@YossiSaadi YossiSaadi marked this pull request as ready for review April 9, 2024 16:23
@YossiSaadi YossiSaadi requested a review from a team as a code owner April 9, 2024 16:23
@YossiSaadi YossiSaadi marked this pull request as draft April 9, 2024 17:24
Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Super cool! Do you think it's worth adding stories (without doc page) so it will be chromaticated and hidden from SB?

Copy link
Member

Choose a reason for hiding this comment

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

I'd call it just Input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm let's discuss it
I'm leaving it open for having an Input component which would be exported from Vibe

packages/core/src/components/BaseInput/BaseInput.types.ts Outdated Show resolved Hide resolved
@YossiSaadi
Copy link
Contributor Author

added ability to hide stories here #2062
make sure to merge this only after #2062 is merged

@YossiSaadi YossiSaadi marked this pull request as ready for review April 15, 2024 08:27
@YossiSaadi YossiSaadi requested a review from talkor April 15, 2024 08:45
@YossiSaadi YossiSaadi force-pushed the chore/yossi/reusable-base-input branch from 05dfdc2 to 584dfa2 Compare April 15, 2024 10:13
Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Very cool!

}
}

&:has(.input[aria-invalid="true"]) {
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we say we can't use has for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added support for both browsers that do and do not support has

describe("with declared props", () => {
it("should apply the size class", () => {
const { getByLabelText } = renderBaseInput({ size: "large" });
expect(getByLabelText("base-input").parentNode).toHaveClass("large");
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use container or something similar to get the outer element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

container gives where the BaseInput is rendered, so I need to query the there the firstChild so it is pretty much the same..
I'll leave it the way it is I think

@YossiSaadi YossiSaadi force-pushed the chore/yossi/reusable-base-input branch from ec1ceda to 82d9597 Compare April 17, 2024 10:19
@YossiSaadi YossiSaadi merged commit c694d7d into master Apr 17, 2024
9 of 10 checks passed
@YossiSaadi YossiSaadi deleted the chore/yossi/reusable-base-input branch April 17, 2024 11:14
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