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

docs(StepGroup): API Decision #2124

Merged
merged 11 commits into from
Apr 25, 2024
Merged

docs(StepGroup): API Decision #2124

merged 11 commits into from
Apr 25, 2024

Conversation

saurabhdaware
Copy link
Member

@saurabhdaware saurabhdaware commented Apr 12, 2024

Copy link

changeset-bot bot commented Apr 12, 2024

⚠️ No Changeset found

Latest commit: 2ba0aec

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Apr 12, 2024

✅ PR title follows Conventional Commits specification.

Copy link

codesandbox-ci bot commented Apr 12, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2ba0aec:

Sandbox Source
razorpay/blade: basic Configuration

@rzpcibot
Copy link
Collaborator

rzpcibot commented Apr 12, 2024

Bundle Size Report

No bundle size changes detected.

Generated by 🚫 dangerJS against 2ba0aec


```jsx
const InteractiveStepGroup = () => {
const [selectedIndex, setSelectedIndex] = React.useState(-1);
Copy link
Member

Choose a reason for hiding this comment

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

How to handle interactive + nested step group?

Copy link
Member Author

Choose a reason for hiding this comment

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

this index is anyway consumer-side only so they can either just give 1, 2, 3, 4, indexes in sequential manner indepdent of nesting and handle

Or they can keep state like useState('1-2') to represent second item in first StepGroup. Either way eventually they just have to give isSelected to selected item somehow

| size | Size of StepGroup | 'medium' \| 'large' | 'medium' |
| orientation | Orientation of StepGroup | 'horizontal' \| 'vertical' | 'vertical' |

### StepItem
Copy link
Member

Choose a reason for hiding this comment

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

Where's children for StepItem?

Slot ka content kaha jayga?

Copy link
Member Author

Choose a reason for hiding this comment

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

ops forgot to add in this table. will add


- ### Should we add `isInteractive` prop?

In the proposed API, I have proposed that we can turn item into interactive or static based on whether it has `onClick` or `href` or nothing. Is it intuitive enough? or should we add more explict prop called `isInteractive` like we have in design
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 this is Okay, similar thing we are doing on Card too. where adding href/onClick makes the card interactive without an explicit prop.


- ### Alternative to `leading`

Currently I have proposed `leading` prop where we can add Icon or indicator. Although in horizontal orientation, its not exactly "leading". It comes on top. Its also not very equivalent to leading we have in other components.
Copy link
Member

Choose a reason for hiding this comment

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

what happens to trailing on horizontal variant?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no trailing in horizontal

</StepGroup>
```

Q. Should we go with 1st, 2nd, or 3rd approach?
Copy link
Member

Choose a reason for hiding this comment

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

3rd approach has a slightly better DX. With the first approach I'll have to manually pass isSelected prop to each nested step group.

But the 1st approach is a bit more flexible, imagine if user wants to add some other conditional logic on isSelected with 1st approach they can.

<StepItem isSelected={!orderExpired && selectedIndex === 3} />

or have multiple selected? If we even want to support it

<StepItem isSelected={selectedIndex.includes(1)} />
<StepItem isSelected={selectedIndex.includes(2)} />

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup I am also leaning towards 1st one because of flexibility. I don't think so multiple selections is a usecase but that additional condition seems fair.

Also biggest reason is if we go with 3rd or 2nd, we have to define how we want to handle nested + interactive. So might have to define some strange index like selected="1-2"

Which is not the concern in 1st - #2124 (comment)

@saurabhdaware saurabhdaware marked this pull request as ready for review April 18, 2024 06:38
Copy link
Member

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM

@saurabhdaware saurabhdaware merged commit e25647d into master Apr 25, 2024
17 checks passed
@saurabhdaware saurabhdaware deleted the stepper/step-0 branch April 25, 2024 04:49
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