Slider - mergestyle conversion#5379
Conversation
| } | ||
|
|
||
| const getClassNames = classNamesFunction<ISliderStyleProps, ISliderStyles>(); | ||
| @customizable('Label', ['theme', 'styles']) |
There was a problem hiding this comment.
I thought we were no longer using this decorator? Styled is doing the same thing...
| rootIsDisabled: disabled, | ||
| rootIsEnabled: !disabled, | ||
| rootIsVertical: vertical, | ||
| rootIsHorizontal: !vertical, |
There was a problem hiding this comment.
I know it was in original code, but isn't it enough to just have one of each of these variables in the styles props interface? Seems redundant...
There was a problem hiding this comment.
um very much agreed. I'm gonna clean this up
| props.rootIsVertical && { | ||
| marginRight: 8 | ||
| }, | ||
| className, |
There was a problem hiding this comment.
Provided className should be listed last to make sure it gets priority in styling... also, is there a test for a provided className?
| className= | ||
| ms-Slider | ||
| ms-Slider-enabled | ||
| ms-Slider-row |
There was a problem hiding this comment.
Two undefineds in original seem to imply custom classNames.. I'd recommend modifying the test to supply them so they show up in the output
There was a problem hiding this comment.
Actually these undefined's were actually extraneos because they are actually toggles of each other (disabled vs enabled, etc).
| { | ||
| "packageName": "office-ui-fabric-react", | ||
| "comment": "Convert Slider to merge-styles", | ||
| "type": "patch" |
There was a problem hiding this comment.
I think this should be minor since interfaces changed
| rootIsVertical?: boolean; | ||
| showTransitions?: boolean; | ||
| showValue?: boolean; | ||
| } |
There was a problem hiding this comment.
I know this was already here but we've been doing something like this to more tightly bind it to the component's props interface:
export type ITeachingBubbleStyleProps =
Required<Pick<ITeachingBubbleProps, 'theme'>> &
Pick<ITeachingBubbleProps, 'hasCondensedHeadline' | 'hasSmallHeadline' | 'isWide'> & {
/** Class name for callout. */
'calloutClassName'?: string
};
| width: 30, | ||
| lineHeight: '1' // using a string here meaning it's relative to the size of the font | ||
| }, | ||
| props.vertical |
There was a problem hiding this comment.
Just a nit, but why not destructure all of these?
* master: (151 commits) Css updates for card components (microsoft#5393) DatePicker MergeStyles step 2 - Converts scss to js styles (microsoft#5362) Update stale with 60 day comment and updated ignored label names. (microsoft#5388) replaced await for ie compat syntax - but still needs promise (microsoft#5387) Slider - mergestyle conversion (microsoft#5379) Applying package updates. Enabling publishing for charting and gridlayout. (microsoft#5378) TilesList: fadeOut overlay. (microsoft#5381) Pivot: JS Styling (microsoft#5324) ShimmeredDetailsList: wrapper for DetailsList with Shimmer. (microsoft#5374) dashboard-grid-layout improved example (microsoft#5373) Applying package updates. Addressing Issue microsoft#5165 - Using Customizer with Nav Component (microsoft#5361) Applying package updates. Address issue microsoft#5353 (microsoft#5359) Change log to trigger release (microsoft#5358) Fluent updates (microsoft#5357) TextField render value undefined as empty string (microsoft#5349) Applying package updates. DetailsColumn: css class changes to show gripper when hover on draggable columns (microsoft#5309) ...
3 participants
Pull request checklist
$ npm run changeDescription of changes
This converts slider to use mergestyles
Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow