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

add button element props #227

Merged
merged 1 commit into from
Apr 12, 2019
Merged

add button element props #227

merged 1 commit into from
Apr 12, 2019

Conversation

l3satwik
Copy link
Contributor

@l3satwik l3satwik commented Apr 9, 2019

No description provided.

@l3satwik l3satwik requested review from azizhk and ritz078 April 9, 2019 10:29
@codecov
Copy link

codecov bot commented Apr 12, 2019

Codecov Report

Merging #227 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #227   +/-   ##
=======================================
  Coverage   82.24%   82.24%           
=======================================
  Files          75       75           
  Lines        1098     1098           
  Branches      201      201           
=======================================
  Hits          903      903           
  Misses        174      174           
  Partials       21       21
Impacted Files Coverage Δ
src/components/Button.tsx 82.6% <100%> (ø) ⬆️

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 93cfe79...ba07e5f. Read the comment docs.

@ritz078 ritz078 merged commit 7894cd8 into master Apr 12, 2019
@ritz078 ritz078 deleted the button-props branch April 12, 2019 08:39
Copy link
Contributor

@azizhk azizhk left a comment

Choose a reason for hiding this comment

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

Forgot to submit

@@ -15,6 +15,7 @@ export interface ButtonProps {
showRipple?: boolean;
loading?: boolean;
filled?: boolean;
buttonProps?: React.ButtonHTMLAttributes<HTMLButtonElement>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't take in extra object prop.
Allow them to be passed in props level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it will cause conflicts, for e.g., type of HTMLButtonElement means and accepts value other than what type of Pebble's Button component.

@azizhk
Copy link
Contributor

azizhk commented Apr 12, 2019

I thought I had submitted

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