Button: Creating ActionButton variant and cleaning styling #8645
Conversation
|
Size Auditor did not detect a change in bundle size for any component! |
|
|
||
| export interface IButtonVariantProps extends IButtonProps { | ||
| iconProps?: IIconProps; |
There was a problem hiding this comment.
I think we should consider marking this prop as deprecated in master and then remove it for Fabric 7 release, which would mean we wouldn't need to add it here.
There was a problem hiding this comment.
But how can we mark it as deprecated in the current Button in oufr if we still don't offer an alternative in that production component?
There was a problem hiding this comment.
By adding a content prop.. should be relatively easy. Unless we don't think that's a good permanent name for Button content, in which case we have a larger problem to address.
There was a problem hiding this comment.
And then we do the same for iconProps? Deprecate it in favor of a new icon prop?
There was a problem hiding this comment.
Possibly. It's either that or we carry over duplicate names for props even in Fabric 7 which seems cumbersome to take on for all of 7.x. I wonder what @dzearing thinks.
|
|
||
| return <Button primary content={text} {...rest} />; | ||
| return <Button primary content={text} icon={iconProps} {...rest} />; |
There was a problem hiding this comment.
What is the name that appears in the hierarchy for these variants? If it's Button then styling and theming targeting PrimaryButton by name won't work. If both Button and PrimaryButton are appearing in hierarchy, then I think styling and theming will be even more wonky.
All of this is just driving need for an extendComponent utility that will just have one entry called PrimaryButton in the hierarchy that should still take directed styling and theming. This is covered by #8331 (and it seems should be resolved before Fabric 7 release.)
There was a problem hiding this comment.
Yes, I think only Button appears in the hierarchy right now.
There was a problem hiding this comment.
The other button variants appear as Unknown. We could apply displayName to them but that won't fix the styling issue since there is no helper like styled reading the setting and applying it (the variants are just thin wrappers.)
I'll add a note to #8331. The only other solution I can think of is to create new variants using styled or createComponent.
| @@ -3,7 +3,7 @@ import { Button } from './Button'; | |||
| import { ButtonVariantsType } from './ButtonVariants.types'; | |||
|
|
|||
| export const DefaultButton: ButtonVariantsType = props => { | |||
| const { text, ...rest } = props; | |||
| const { text, iconProps, ...rest } = props; | |||
There was a problem hiding this comment.
I didn't catch this in the other PR but maybe text is another prop that could be deprecated in master and removed in Fabric 7.
|
Hello @kkjeer! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me and give me an instruction to get started! Learn more here. |
|
@msft-github-bot require approval from @JasonGore and @dzearing |
|
Hello @kkjeer! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
|
@msft-github-bot forget everything |
|
Hello @khmakoto! Because you've told me to reset the custom auto-merge settings, I'll use the configured settings for this repository when I'm merging this pull request. |
|
@msft-github-bot require approval from @JasonGore |
|
Hello @khmakoto! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
|
🎉 Handy links: |
Pull request checklist
$ npm run changeDescription of changes
This PR adds an
ActionButtonvariant to improve parity between new and oldButtoncomponents while also making some styling fixes in general.Below is a comparison, on the left using the
ActionButtonsinoffice-ui-fabric-reactand on the right using the ones inexperiments:Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow