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

feat: [IOBP-236] ListItemSwitch extension with loading state and badge #79

Merged
merged 11 commits into from
Sep 25, 2023

Conversation

Hantex9
Copy link
Contributor

@Hantex9 Hantex9 commented Sep 21, 2023

Short description

This PR adds three more props to ListItemSwitch component:

  • isLoading: while true it shows an activity indicator
  • badgeText: A text to show inside a badge instead of the switch. If it is not null, you have to provide also a badgeVariant
  • badgeVariant: It indicates the badge variant to show;

List of changes proposed in this pull request

  • Added three new props, the badgeText and badgeVariant are conditional properties;
  • Changed the position of the right component and aligned to the title;
  • Added more examples about ListItemSwitch inside example app;

How to test

Open the example app, navigate through List items section and check the new examples:

Preview

listitemswitch.mov

@Hantex9 Hantex9 requested review from dmnplb and a team as code owners September 21, 2023 15:38
Co-authored-by: Cristiano Tofani <cri.tofani@gmail.com>
@Hantex9 Hantex9 requested a review from CrisTofani September 21, 2023 20:51
@CrisTofani
Copy link
Contributor

@dmnplb I have doubt about this implementation: the badge version of the list item should be explicit of this specific list item or be took apart as a scoped component?

@Hantex9
Copy link
Contributor Author

Hantex9 commented Sep 22, 2023

@dmnplb I have doubt about this implementation: the badge version of the list item should be explicit of this specific list item or be took apart as a scoped component?

For any doubts about this component, this is the figma where this component is used. I think that is overkill to create another component only to handle a badge on the right side and it will have negative effect on the DX side

@CrisTofani
Copy link
Contributor

CrisTofani commented Sep 22, 2023

@dmnplb I have doubt about this implementation: the badge version of the list item should be explicit of this specific list item or be took apart as a scoped component?

For any doubts about this component, this is the figma where this component is used. I think that is overkill to create another component only to handle a badge on the right side and it will have negative effect on the DX

Looking to the figma you linked it could even be represented by another component (eg: A standard ListItem with a badge on the right) it doesn't look to be exclusively a ListItemSwitch with the badge rendered conditionally to the Switch element.

I would prefer to keep the ListItemSwitch closed to it's purpose

@dmnplb
Copy link
Collaborator

dmnplb commented Sep 22, 2023

Looking to the figma you linked it could even be represented by another component (eg: A standard ListItem with a badge on the right) it doesn't look to be exclusively a ListItemSwitch with the badge rendered conditionally to the Switch element.

Unfortunately, we can't use this approach because a simple ListItem does not look like a ListItemSwitch without a badge. It has different styles for title and description. In this particular case, we want to have consistent styles between the different cases when we compose them on a single page.

So for me, extending the ListItemSwitch may be a viable option. Fortunately, we don't have to manage the pressed behavior for the entire block like other Selection components.

@Hantex9 Hantex9 requested a review from dmnplb September 22, 2023 10:24
Copy link
Contributor

@CrisTofani CrisTofani left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@dmnplb dmnplb left a comment

Choose a reason for hiding this comment

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

I'll approve it when the components are located in the right section of the example playground (under Selection, instead of List Items)

@Hantex9
Copy link
Contributor Author

Hantex9 commented Sep 25, 2023

I'll approve it when the components are located in the right section of the example playground (under Selection, instead of List Items)

The suggested hint is already addressed into commit 71b2183 on Friday, please take a look at it

@dmnplb
Copy link
Collaborator

dmnplb commented Sep 25, 2023

@Hantex9 My bad, I forgot to pull the latest updates 😅

@Hantex9 Hantex9 merged commit 39ed6c9 into main Sep 25, 2023
@Hantex9 Hantex9 deleted the IOBP-236-list-item-switch-loading-and-badge branch September 25, 2023 07:55
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.

3 participants