feat: stepper component added #523

Waishnav · 2024-11-07T07:20:55Z

No description provided.

netlify · 2024-11-07T07:21:11Z

✅ Deploy Preview for kobalte ready!

Name	Link
🔨 Latest commit
🔍 Latest deploy log	https://app.netlify.com/sites/kobalte/deploys/673bbae8938ade42f8530ccc
😎 Deploy Preview	https://deploy-preview-523--kobalte.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

issues

jer3m01

Thanks for the PR!

There's a few changes needed around accessibility, mostly using a <Tabs> component internally instead of re-implementing them.

packages/core/src/stepper/stepper-item.tsx

packages/core/src/stepper/stepper-prev-trigger.tsx

packages/core/src/stepper/stepper-separator.tsx

packages/core/src/stepper/stepper-trigger.tsx

packages/core/src/stepper/stepper-next-trigger.tsx

packages/core/src/stepper/stepper-item.tsx

packages/core/src/stepper/stepper-list.tsx

packages/core/src/stepper/stepper-root.tsx

packages/core/src/stepper/stepper-content.tsx

Waishnav · 2024-11-13T12:59:12Z

Thanks for the PR!

There's a few changes needed around accessibility, mostly using a <Tabs> component internally instead of re-implementing them.

Thanks for the detailed review. I've push the new commit and used the Tabs component internally. could you please go through new changes.

hngngn · 2024-11-16T10:21:30Z

The previous button is not functioning correctly. When at step 1, it works as expected. However, when navigating to step 2 and then clicking previous, instead of returning to step 1, it moves to step 3.

Waishnav · 2024-11-16T11:03:28Z

The previous button is not functioning correctly. When at step 1, it works as expected. However, when navigating to step 2 and then clicking previous, instead of returning to step 1, it moves to step 3.

its old deployment, I've fixed it in new commits.

zobweyt · 2024-11-17T11:37:46Z

packages/core/src/stepper/stepper-next-trigger.tsx

+	const [local, others] = splitProps(props as StepperNextTriggerProps, ["onClick"]);
+
+	const onClick: JSX.EventHandlerUnion<HTMLButtonElement, MouseEvent> = (e) => {
+		context.setStep(context.step() + 1);


It would be nice if you had context.moveForward or context.moveNext instead. It would be more encapsulated and straightforward and could be used more efficiently by end users.

Also consider adding something like context.canMoveForward or context.canMoveNext and call them in context.moveForward or context.moveNext so the logic can't be broken.

In addition, you will be able to directly use onClick={context.moveForward} or onClick={context.moveNext}.

zobweyt · 2024-11-17T11:39:23Z

packages/core/src/stepper/stepper-prev-trigger.tsx

+	const [local, others] = splitProps(props as StepperPrevTriggerProps, ["onClick"]);
+
+	const onClick: JSX.EventHandlerUnion<HTMLButtonElement, MouseEvent> = (e) => {
+		if (context.step() > 0) {


It would be nice if you had context.moveBackward or context.movePrevious instead. It would be more encapsulated and straightforward and could be used more efficiently by end users.

Also consider adding something like context.canMoveBackward or context.canMovePrevious and call them in context.moveBackward or context.movePrevious so the logic can't be broken.

In addition, you will be able to directly use onClick={context.moveBackward} or onClick={context.movePrevious}.

zobweyt · 2024-11-17T11:40:49Z

packages/core/src/stepper/stepper-next-trigger.tsx

+		context.setStep(context.step() + 1);
+	};
+
+	const isDisabled = () => context.isDisabled() || context.isCompleted();


If you add something like context.canMoveForward or context.canMoveNext, add it here too.

zobweyt · 2024-11-17T11:41:05Z

packages/core/src/stepper/stepper-prev-trigger.tsx

+		}
+	};
+
+	const isDisabled = () => context.isDisabled() || context.step() === 0;


If you add something like context.canMoveBackward or context.canMovePrevious, add it here too.

zobweyt · 2024-11-17T11:43:04Z

packages/core/src/stepper/stepper-prev-trigger.tsx

+	const isDisabled = () => context.isDisabled() || context.step() === 0;
+
+	return (
+		<Show when={!context.isCompleted()}>


Shouldn't this logic be handled directly by end users? What if I still want to see these buttons and view all my steps?

zobweyt · 2024-11-17T11:55:02Z

packages/core/src/stepper/stepper-root.tsx

+	disabled?: boolean;
+
+  /** The maximum number of steps in the stepper. */
+  maxSteps: number;


Can steps be added dynamically? If so, the term maxSteps may not be appropriate. I would suggest using terms like stepsCount or length instead.

are you suggesting, to handle this maxSteps count internally instead of having this as prop? or suggesting it to change it's name

In my opinion, it would be more effective to manage the maxSteps count internally rather than exposing it as a prop. This approach will minimize the potential for human errors and simplify the component overall. You might consider adding context.register in the onMount and context.unregister in the onCleanup method in the Stepper.Content and maybe Stepper.CompletedContent. These functions can be used to increment or decrement the stepsCount or length prop accordingly.

zobweyt · 2024-11-17T11:59:14Z

packages/core/src/stepper/stepper-content.tsx

Isn't the term StepperStep more suitable than StepperContent?

Additionally, can you determine if the current step is active within this component? It would also be helpful to listen for an event when the current step becomes active, especially in complex multi-step forms.

zobweyt · 2024-11-17T12:03:55Z

packages/core/src/stepper/stepper-content.tsx

+import { type ElementOf, type PolymorphicProps } from "../polymorphic";
+
+export interface StepperContentOptions {
+  index: number;


Can this index be calculated dynamically? For instance, if I don't need the step triggers at the top and want to have dynamic steps?

To accomplish this, I would use a "register and unregister" pattern to adjust the index dynamically and set it for the tab content.

See for example

https://github.com/corvudev/corvu/blob/e87c758a5bc64cb81d7a7900bd3ae99abaeb298f/packages/accordion/src/context.ts#L65-L66

https://github.com/corvudev/corvu/blob/e87c758a5bc64cb81d7a7900bd3ae99abaeb298f/packages/accordion/src/Root.tsx#L243-L246

Usually, stepper forms are not dynamic and have a fixed number of steps. So, I thought it would be better not to consider this edge case.

Your approach is quite reasonable. In JSX, you can place step components anywhere and manually set their indexes.

However, let's consider a scenario where you have a room booking form. A room may or may not have zones (i.e. it's large or not), and these zones can also be booked.

In this case, if you have a room selection step and want to dynamically add the next step based on whether the selected room has zones, would it be more effective to implement a regular step skip (i.e. context.setCurrentIndex(context.currentIndex + 2)) or to dynamically add a new step?

Additionally, how would this look if we use Stepper.Triggers for the steps? How will it be displayed visually? Let's say empty or not completed? In dynamic implementation there are also disadvantages for this: UI can "jump"

zobweyt · 2024-11-17T12:06:48Z

packages/core/src/stepper/stepper-prev-trigger.tsx

+
+	const onClick: JSX.EventHandlerUnion<HTMLButtonElement, MouseEvent> = (e) => {
+		if (context.step() > 0) {
+			context.setStep(context.step() + 1);


Suggested change

context.setStep(context.step() + 1);

context.setStep(context.step() - 1);

Ah, my bad! I think that was a typo—thanks for catching it!

Yeah! That's why I suggested adding functions like moveForward and moveBackward to the context. You'll reduce the number of human typos like this!

zobweyt · 2024-11-17T12:08:54Z

apps/docs/src/examples/stepper.tsx

What do you think about adding another example without step triggers? I think it will be useful for some people

zobweyt · 2024-11-18T14:30:46Z

Why are there two components Stepper.Item and Stepper.Trigger? Can't they be combined into one? Stepper.Item also gets confused with Stepper.Content

zobweyt · 2024-11-18T14:32:37Z

In the form example the "Enter" key is ignored. I think this is an accessibility issue

zobweyt · 2024-11-18T14:41:05Z

What do you think about introducing something like onBeforeStepChange? This will allow us to handle step validation easier. We could listen to this event inside a specific step component and compare the current index with the step index. This way we can move all the validation logic to each individual step, which will help us eliminate long components. As a result, we will wrap each step component in a separate wrapper. This is very useful in complex forms.

Or you can add events to the step component directly, so you don't need to compare the index each time. This is a cool feature for end users and extensibility.

By using onBeforeStepChange you can cancel navigation programmatically and thus indicate that something is wrong with the current step.

feat: stepper component added

f3b2f9b

Waishnav added 2 commits November 10, 2024 17:07

fix: written more similar to pagniation component and fix some type

d5d4996

issues

added documentation for the stepper component

e9e3c20

Waishnav marked this pull request as ready for review November 10, 2024 18:51

Waishnav mentioned this pull request Nov 10, 2024

New component - Stepper #520

Open

jer3m01 requested changes Nov 12, 2024

View reviewed changes

Waishnav added 2 commits November 13, 2024 17:56

fix: use Tabs instead of reimplementing it

48976ac

fix: docs upadted with basic example as uncontrolled and other examples

12bc10b

zobweyt suggested changes Nov 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: stepper component added #523

feat: stepper component added #523

Waishnav commented Nov 7, 2024

netlify bot commented Nov 7, 2024 •

edited

Loading

jer3m01 left a comment

Waishnav commented Nov 13, 2024

hngngn commented Nov 16, 2024

Waishnav commented Nov 16, 2024

zobweyt Nov 17, 2024

zobweyt Nov 17, 2024

zobweyt Nov 17, 2024

zobweyt Nov 17, 2024

zobweyt Nov 17, 2024

zobweyt Nov 17, 2024

Waishnav Nov 18, 2024

zobweyt Nov 18, 2024

zobweyt Nov 17, 2024

zobweyt Nov 17, 2024

Waishnav Nov 18, 2024 •

edited

Loading

zobweyt Nov 18, 2024

zobweyt Nov 17, 2024

Waishnav Nov 18, 2024

zobweyt Nov 18, 2024

zobweyt Nov 17, 2024

zobweyt commented Nov 18, 2024

zobweyt commented Nov 18, 2024

zobweyt commented Nov 18, 2024 •

edited

Loading

	context.setStep(context.step() + 1);
	context.setStep(context.step() - 1);

feat: stepper component added #523

Are you sure you want to change the base?

feat: stepper component added #523

Conversation

Waishnav commented Nov 7, 2024

netlify bot commented Nov 7, 2024 • edited Loading

✅ Deploy Preview for kobalte ready!

jer3m01 left a comment

Choose a reason for hiding this comment

Waishnav commented Nov 13, 2024

hngngn commented Nov 16, 2024

Waishnav commented Nov 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Waishnav Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zobweyt commented Nov 18, 2024

zobweyt commented Nov 18, 2024

zobweyt commented Nov 18, 2024 • edited Loading

netlify bot commented Nov 7, 2024 •

edited

Loading

Waishnav Nov 18, 2024 •

edited

Loading

zobweyt commented Nov 18, 2024 •

edited

Loading