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(Button): add link icon position for link buttons #3798
Conversation
PF3 preview: https://patternfly-react-pr-3798-pf3.surge.sh |
Codecov Report
@@ Coverage Diff @@
## master #3798 +/- ##
=========================================
+ Coverage 71% 71% +<.01%
=========================================
Files 785 785
Lines 10646 10650 +4
Branches 2321 2325 +4
=========================================
+ Hits 7559 7562 +3
Misses 2657 2657
- Partials 430 431 +1
Continue to review full report at Codecov.
|
@@ -95,8 +98,14 @@ const Button: React.FunctionComponent<ButtonProps & InjectedOuiaProps> = ({ | |||
'data-ouia-component-id': ouiaId || ouiaContext.ouiaId | |||
})} | |||
> | |||
{icon && variant === ButtonVariant.link && <span className="pf-c-button__icon">{icon}</span>} | |||
{children} | |||
{icon && variant === ButtonVariant.link && linkPosition === 'left' && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is beyond the scope of this PR since this was the previous behavior, but do we need to restrict use of .pf-c-button__icon
to ButtonVariant.link
? This is probably a question for @mcarrano or @mceledonia. Seems to me like it could be supported in any button, except maybe the plain variation.
Also should we call it iconPosition
instead of linkPosition
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason to restrict it, what do you think @mceledonia ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it should be possible to place the icon on either side of the link label. But can we just make one change to the example @kmcfaul ? Can we replace the "+" icon for this example, only, with something like the external link icon? If this were an add button, it's unlikely we'd want to place the "+" following the label. Thanks.
71a67c7
to
279820b
Compare
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #3283