Slider: Convert to mergeStyles (Part #1)#4796
Conversation
KevinTCoughlin
requested review from
dzearing,
jordandrako,
mikewheaton and
oengusmacinog-zz
| renderedValue?: number; | ||
| } | ||
|
|
||
| export enum ValuePosition { |
There was a problem hiding this comment.
FYI - This seems to be unused and can be deprecated / removed in a subsequent PR.
|
Hmm looks like a snapshot test is failing.. taking a look: FAIL src/components/Slider/Slider.test.tsx (8.835s)
● Slider › renders Slider correctly
expect(value).toMatchSnapshot()
Received value does not match stored snapshot 1.
- Snapshot
+ Received
@@ -10,11 +10,11 @@
aria-valuemin={0}
aria-valuenow={0}
aria-valuetext={undefined}
className="ms-Slider-slideBox ms-Slider-showValue ms-Slider-showTransitions undefined"
disabled={false}
- id="Slider0"
+ id="Slider1"
onKeyDown={[Function]}
onMouseDown={[Function]}
onTouchStart={[Function]}
role="slider"
type="button"
at Object.<anonymous> (src/components/Slider/Slider.test.tsx:15:18)
at new Promise (<anonymous>)
at <anonymous>
● Slider › renders a slider
TypeError: Cannot read property 'querySelector' of null
at Object.<anonymous> (src/components/Slider/Slider.test.tsx:23:38)
at new Promise (<anonymous>)
at <anonymous>
● Slider › can slide to default min/max and execute onChange
TypeError: Cannot read property 'querySelector' of null
at Object.<anonymous> (src/components/Slider/Slider.test.tsx:40:36)
at new Promise (<anonymous>)
at <anonymous>
● Slider › has type=button on all buttons
TypeError: Cannot read property 'querySelectorAll' of null
at Object.<anonymous> (src/components/Slider/Slider.test.tsx:77:36)
at new Promise (<anonymous>)
at <anonymous>
● Slider › renders correct aria-valuetext
TypeError: Cannot read property 'querySelector' of null
at Object.<anonymous> (src/components/Slider/Slider.test.tsx:101:30)
at new Promise (<anonymous>)
at <anonymous>
› 1 snapshot test failed. |
|
Thank you @oengusmacinog for the snapshots walk-thru! |
| it('renders a slider', () => { | ||
| const component = ReactTestUtils.renderIntoDocument( | ||
| <Slider label='slider' /> | ||
| const component = ReactTestUtils.renderIntoDocument<SliderBase>( |
There was a problem hiding this comment.
Note: that I've refactored the tests to test <SliderBase/> as we did in 3626565
There was a problem hiding this comment.
I really don't completely understand this. What is the reasoning?
There was a problem hiding this comment.
I was trying to follow a component's tests that followed the getStyles pattern and were passing. Seems inconsistent tho and perhaps I followed the wrong pattern. Prior to using <SliderBase/> the DOM wouldn't render, likely because I was omitting something that was causing <Slider/> to throw.
There was a problem hiding this comment.
Re: the type parameter changes those can be removed and won't affect anything, just happened to be the pattern that I followed from 3626565 but even that is inconsistent.
jordandrako
left a comment
jordandrako
left a comment
There was a problem hiding this comment.
LGTM, a few comments.
| } | ||
|
|
||
| export class SliderBase extends BaseComponent<ISliderProps, ISliderState> implements ISlider { | ||
| public static defaultProps: {} = { |
There was a problem hiding this comment.
For next PR (assuming this PR won't contain transitioning from sass to mergeStyles in the base component file): Type of defaultProps should probably be ISliderProps instead of {}.
There was a problem hiding this comment.
I think this is a worthwhile change to make now since it could make for some hard to find bugs later. In general {} should never be used as a type.
| * Theme provided by High-Order Component. | ||
| */ | ||
| theme?: ITheme; | ||
|
|
There was a problem hiding this comment.
Add className?: string; as well so consumers can add a class to the root component.
There was a problem hiding this comment.
className? is already available its just lower in the field set
|
|
||
| return ( | ||
| <div | ||
| className={ css('ms-Slider', styles.root, className, { |
There was a problem hiding this comment.
For next PR: Make sure all this logic is in the styles file, including the globalClassNames such as "ms-Slider-enabled" etc. There shouldn't be a need for the css() function anywhere.
|
@JasonGore Kevin is out on leave; can we get this merge conflict fixed and clean up? Not urgent but doesn't look too bad to help out. |
6 participants
Pull request checklist
$ npm run changeDescription of changes
Part 1 of converting
<Slider/>to mergeStyles, changes include:.styles.tsfile.Slider.tsxintoSlider.base.tsxand styledSlider.tsx.getGlobalClassNamesintroduced by Flag on theme for no global class names #4635I used the following as references (first time doing this type of refactor):
Focus areas to test
That the
<Slider/>renders as expected w/ its styles.