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

test(Slider): CSF 3 & visual testing #1614

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

itwillwork
Copy link
Contributor

No description provided.

@itwillwork itwillwork changed the title refactor(Slider): migrate CSF3 refactor(Slider): CSF 3 & visual testing May 23, 2024
@itwillwork itwillwork changed the title refactor(Slider): CSF 3 & visual testing [WIP] refactor(Slider): CSF 3 & visual testing May 23, 2024
@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@gravity-ui-bot
Copy link
Contributor

Playwright Test Component is ready.

@itwillwork itwillwork changed the title [WIP] refactor(Slider): CSF 3 & visual testing refactor(Slider): CSF 3 & visual testing May 23, 2024
@itwillwork itwillwork changed the title refactor(Slider): CSF 3 & visual testing test(Slider): CSF 3 & visual testing May 23, 2024
@Arucard89
Copy link
Contributor

I suggest grouping single and range components in one page for each case. This will help reduce the number of menu items in the showcase section. I think it will be more convenient for watching.

@amje
Copy link
Contributor

amje commented Jun 4, 2024

I suggest grouping single and range components in one page for each case. This will help reduce the number of menu items in the showcase section. I think it will be more convenient for watching.

Agreed, can we write the stories so we have both single and range slider, but with shared controls.value control will be the value for the single slider and rangeValue control will be the value for the ranged slider?

return (
<div className={b(null, className)}>
{title && <div className={b('title')}>{title}</div>}
{description && <div className={b('description')}>{description}</div>}
<div className={b('content')}>{children}</div>
<div className={b('content', {isVertical})}>{children}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I've added direction prop in the recent update, you can use it instead

@@ -13,7 +15,7 @@ import type {
interface ComponentFixtures {
mount<HooksConfig extends JsonObject>(
component: JSX.Element,
options?: MountOptions<HooksConfig>,
options?: MountOptions<HooksConfig> & {wrapDivStyles?: React.CSSProperties},
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
options?: MountOptions<HooksConfig> & {wrapDivStyles?: React.CSSProperties},
options?: MountOptions<HooksConfig> & {wrapperStyle?: React.CSSProperties},

@Arucard89
Copy link
Contributor

Arucard89 commented Jun 5, 2024

I suggest grouping single and range components in one page for each case. This will help reduce the number of menu items in the showcase section. I think it will be more convenient for watching.

Agreed, can we write the stories so we have both single and range slider, but with shared controls.value control will be the value for the single slider and rangeValue control will be the value for the ranged slider?

@amje, Maybe we could omit the value property altogether? I don't think it is really necessary for the showcases(excluding the default showcase).

UPD:
So my point is to have:
-single page for slider(with value prop)
-single page for range slider(with value prop)
-single and range components in one page for each other special case(without value prop)

@amje
Copy link
Contributor

amje commented Jun 5, 2024

I suggest grouping single and range components in one page for each case. This will help reduce the number of menu items in the showcase section. I think it will be more convenient for watching.

Agreed, can we write the stories so we have both single and range slider, but with shared controls.value control will be the value for the single slider and rangeValue control will be the value for the ranged slider?

@amje, Maybe we could omit the value property altogether? I don't think it is really necessary for the showcases(excluding the default showcase).

UPD: So my point is to have: -single page for slider(with value prop) -single page for range slider(with value prop) -single and range components in one page for each other special case(without value prop)

@Arucard89 Like this?
Default (single default with value)
Range (range default with value)
Size (both single and range without value)
Disabled (both single and range without value)
// etc

@Arucard89
Copy link
Contributor

@Arucard89 Like this?
Default (single default with value)
Range (range default with value)
Size (both single and range without value)
Disabled (both single and range without value)
// etc

@amje, yes. I believe that it would be helpful for users to play around with the components using the default settings and explore the special, predefined cases.

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.

4 participants