CalloutContent: Convert CalloutContent to JS Styling#3745
Conversation
This reverts commit d5a842d.
…fice-ui-fabric-react into malozano/CalloutStyling
| maskOrigin?: ICSSRule | string; | ||
| maxFontSize?: ICSSRule | string; | ||
| maxHeight?: ICSSRule | string; | ||
| maxFontSize?: ICSSRule | ICSSPixelUnitRule; |
There was a problem hiding this comment.
I changed maxFontSize and maxHeight because components have these props set as numbers, so with ICSSPixelUnitRule it can be either a number and string (the same we do for other length css rules). Just wanted to verify it is all right with you guys and there are no concerns?
| */ | ||
| theme: ITheme; | ||
|
|
||
| calloutWidth?: number; |
There was a problem hiding this comment.
Will add comments to all the props
| /** | ||
| * Generates style to clear browser specific focus styles. | ||
| */ | ||
| export function focusClear(): IRawStyle { |
There was a problem hiding this comment.
focusClear is a mixin used by several components so I created a function in the styling package. I just realized that there is one in the experiments package. What are your thoughts? Should I remove the one from the experimental package if we keep this hear?
| * Deprecated at v0.59.1, to be removed at >= v1.0.0. Pass in a beakWidth to dictate size. | ||
| * @deprecated | ||
| */ | ||
| beakStyle?: string; |
There was a problem hiding this comment.
This has been deprecated in the props, but is still being used in the getBeakStyle func. Is it safe to remove from props, styleprops and func or are there still concerns of this being breaking change? Comment makes me think that it is okay to remove now
There was a problem hiding this comment.
It's probably not, we should remove it in 6.0, although I really hope no one is using it right now...
| exports[`Callout renders Callout correctly 1`] = ` | ||
| <div | ||
| className="ms-Callout-container" | ||
| className={undefined} |
There was a problem hiding this comment.
some of the global class names are being dropped off here. Is this being tested correctly? You shouldn't see a big change in the snapshot tests.
There was a problem hiding this comment.
What do you mean by tested correctly, unit tests?
There was a problem hiding this comment.
I thing this happened because I used CalloutContentBase in the tests instead of CalloutContent. After changing that, I get the classes back.
| filter: 'opacity(0)', | ||
| }, | ||
| positions && { | ||
| top: positions.elementPosition.top, |
There was a problem hiding this comment.
ITT You can just do positions && positions.elementPosition, since it's already got bottom, left, right and such. You might need to expand it with ... or something though.
There was a problem hiding this comment.
positions && { ...positions.elementPosition }
There was a problem hiding this comment.
Let me make this change
| maskOrigin?: ICSSRule | string; | ||
| maxFontSize?: ICSSRule | string; | ||
| maxHeight?: ICSSRule | string; | ||
| maxFontSize?: ICSSRule | ICSSPixelUnitRule; |
| * Deprecated at v0.59.1, to be removed at >= v1.0.0. Pass in a beakWidth to dictate size. | ||
| * @deprecated | ||
| */ | ||
| beakStyle?: string; |
There was a problem hiding this comment.
It's probably not, we should remove it in 6.0, although I really hope no one is using it right now...
| /** | ||
| * Generates style to clear browser specific focus styles. | ||
| */ | ||
| export function focusClear(): IRawStyle { |
4 participants
Pull request checklist
$ npm run changeDescription of changes
Created CalloutContentBase and separated the styling of the CalloutContent to a getStyles function. This will enable customization of the Callout component.
Change also adds the mixin focus-clear to the styling package, so it is reusable for other components.
Focus areas to test
Screen shots of components should look the same.