🚀 feat(button): add prop loading

[matheushdsbr]

Description 📄

This is a new prop of button, now we can use loading on buttons! ✨

We can use it when we are going to perform some type of loading or request, and it will take a while to finish. That's when the loading comes into action.

Exemple:
<Button isLoading>Find an activity</Button>
<Button isLoading={false}>Find an activity</Button>

Platforms 📲

• Web
• Mobile

Type of change 🔍

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested? 🧪

[Enter the tips to test this PR]

• Unit Test
• Snapshot Test

Checklist: 🔍

• My code follows the contribution guide of this project Contributing Guide
• Layout matches design prototype: FIGMA
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

Screenshots 📸

Default Button

Screen.Recording.2023-10-25.at.18.39.26.mov

Icon Button

Screen.Recording.2023-10-23.at.15.03.14.mov

Outline Button

Screen.Recording.2023-10-23.at.15.03.44.mov

[frgiovanna]

Why not make the isLoading an optional prop for the already existent buttons?

Example:

<Button isLoading={loading} />
<Button.Outline isLoading={loading} />
<Button.Icon isLoading={loading} />

(On one of our projects has an abstraction that does what I am suggesting, you can check the code here and the documentation here - needs VPN)

[joaopaulonsoares-gympass]

I agree with @frgiovanna , have loading as props of the current button would be better than a new component.

[caiotracera]

As said by @frgiovanna and @joaopaulonsoares-gympass, I really think that we should change this, so the loading state can be a prop instead of a whole new component.

[frgiovanna]

Good job so far, please check my comments

[frgiovanna]

🏅

[frgiovanna]

Where are we using this destructured isLoading?

[matheushdsbr]

The original idea was to use all the properties except 'isLoading' when dealing with 'Button.Link' and 'Button.Text.' However, I had to disable eslint since 'isLoading' was not being utilized; it was solely removed to prevent its use in 'Button.Link' and 'Button.Text.'

[frgiovanna]

Do you think is scalable if you get all of the other props except the isLoading? This means every time someone adds a new value for all buttons they will need to add it here too, otherwise it will be ignored. WDYT?

[matheushdsbr]

Good point 🤔

I'm just concerned that someone might misunderstand the 'button.text' and 'button.link' props and try to use them. By removing this approach, people will still be able to use isLoading, and it will work. However, it wouldn't be advisable to do so because, as far as I know, we don't have any plans for that.

But, yes I agree with you. This approach isn't scalable. It's more on the developer's side to use it correctly.

[frgiovanna]

No no, I really got your point and I think you're right, I'm just wondering if we can handle it in another way instead of having this eslint comment

[frgiovanna]

What if you do something like this?

const ButtonLink = forwardRef(({ rest }, ref) => {

const props = Object.fromEntries(
  Object.entries(rest).filter(([key]) => key !== 'isLoading')
);

 return (
   <Link {...props} disabled={props.disabled} aria-disabled={props.disabled} ref={ref} /> 
)

[matheushdsbr]

Great suggestion!! 🚀