Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Button): add loading prop #1662

Merged
merged 16 commits into from
Aug 9, 2019
Merged

feat(Button): add loading prop #1662

merged 16 commits into from
Aug 9, 2019

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Jul 17, 2019

We need to allow user to define loading state of the Buttons. This is a common thing already defined in many UI libraries, like:

Usually, the way it is implemented is, either with showing the loader indicator before the content of the button, or replacing the content of the button with loader:
image
image

In Teams theme, we need the first behavior - showing the loader indicator before the content (instead of the icon).

Proposed API

For showing the loading indicator, we may add loading prop in the Button component, that will show the indicator before the content.

Furthermore, if we want to allow the users to customize this, we may add loaderPosition: 'start' | 'end' | 'center', which will position the loader before, after the content, or replace it if center is provided. Not sure if we want to add this right-away, but it's just an idea of how we may extend the API in the future.

Alternative API

As we already have the icon slot, we may reuse that slot for rendering the Loader component if the loading prop is introduced. Some open questions here are, whether we want to change the component rendered there, in means of instead of Icon.create(...), should we have Loader.create(...) and this applies then, do we want to reuse the icon slot for the Loader's props as well, or introduce additional slot here...

In the styles, conditionally we may apply different margins if the loading prop is provided, but at least the anatomy of the component will be clear.

Blockers

Currently this is blocked by the Loader component, as we don't have two variations for the loader that we may use for default and primary Button. The issues is tracked here: #1663

@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #1662 into master will decrease coverage by 0.02%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1662      +/-   ##
==========================================
- Coverage   71.28%   71.25%   -0.03%     
==========================================
  Files         852      852              
  Lines        7045     7049       +4     
  Branches     2008     2033      +25     
==========================================
+ Hits         5022     5023       +1     
- Misses       2017     2020       +3     
  Partials        6        6
Impacted Files Coverage Δ
.../themes/teams/components/Button/buttonVariables.ts 0% <ø> (ø) ⬆️
...src/themes/teams/components/Button/buttonStyles.ts 3.22% <0%> (-0.23%) ⬇️
packages/react/src/components/Button/Button.tsx 77.77% <85.71%> (-2.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 071c5d4...616bfe7. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #1662 into master will decrease coverage by 0.12%.
The diff coverage is 17.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1662      +/-   ##
==========================================
- Coverage   69.89%   69.76%   -0.13%     
==========================================
  Files         867      867              
  Lines        7338     7353      +15     
  Branches     2147     2158      +11     
==========================================
+ Hits         5129     5130       +1     
- Misses       2201     2215      +14     
  Partials        8        8
Impacted Files Coverage Δ
.../themes/teams/components/Button/buttonVariables.ts 0% <ø> (ø) ⬆️
...src/themes/teams/components/Button/buttonStyles.ts 2.38% <0%> (-0.85%) ⬇️
packages/react/src/components/Loader/Loader.tsx 100% <100%> (ø) ⬆️
packages/react/src/components/Button/Button.tsx 72.41% <40%> (-7.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1381d65...028619c. Read the comment docs.

@mnajdova mnajdova changed the title [WIP] feat(Button): add loading prop feat(Button): add loading prop Jul 17, 2019
manajdov added 2 commits August 8, 2019 12:04
# Conflicts:
#	packages/react/src/components/Button/Button.tsx
#	packages/react/src/themes/teams/components/Button/buttonStyles.ts
@vercel vercel bot temporarily deployed to staging August 8, 2019 12:59 Inactive
@mnajdova mnajdova merged commit 9a984fa into master Aug 9, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/loading-button-prop branch August 9, 2019 08:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants