Convert Check to mergeStyles #3880

jordandrako · 2018-02-03T01:48:14Z

Pull request checklist

Include a change request file using $ npm run change

Description of changes

Move ICheckProps to Check.types.ts file.
Add ICheckStyleProps and ICheckStyles
Convert Check.scss to Check.styles.ts
Remove rootIsChecked class, add checked boolean for conditionals.
Rename Check.tsx to Check.base.tsx and made Check.tsx return a styled CheckBase.
Check can be themed.
Check.scss remains until other components no longer require it as a dependency.

Focus areas to test

Check still looks a behaves the same. Try theming the Check.

Start converting check to mergeStyles

…ked state.

dzearing · 2018-02-06T06:52:40Z

packages/office-ui-fabric-react/src/components/Check/Check.base.tsx

+
+    return (
+      <div
+        className={ css(


css doesn't need to be used and should be removed. Can you move all of the logic to getStyles so that we have it in one place?

gokunymbus · 2018-02-06T20:12:01Z

packages/office-ui-fabric-react/src/components/Check/Check.styles.ts

+
+  const { palette, semanticColors } = theme;
+
+  const _sharedCircleCheck: IStyle = {


nit: We typically only prepend the _ when it's a private member of a class but not a local variable.

Commit pushed

gokunymbus · 2018-02-06T20:13:33Z

packages/office-ui-fabric-react/src/components/Check/Check.styles.ts

+} from '@uifabric/styling';
+import { memoizeFunction } from '@uifabric/utilities';
+
+const DEFAULT_CHECKBOX_HEIGHT: string = '18px';


We shouldn't "hard code" this value unless the consumer will never need to change it. It seems like in this case we should expose this as a public prop.

const { checkBoxHeight = DEFAULT_CHECKBOX_HEIGHT, checked, className, theme, } = props;

Setting checkBoxHeight to equal that variable is just the way to set it as default and is overridden by what the user set in props. To be honest, I didn't need DEFAULT_CHECKBOX_HEIGHT, but I thought it'd be a nice easy way to see/change the default later down the road.

@battletoilet Could you take another look at this review?

gokunymbus · 2018-02-06T20:15:10Z

packages/office-ui-fabric-react/src/components/Check/Check.styles.ts

+    root: [
+      {
+        // lineHeight currently needs to be a string to output without 'px'
+        lineHeight: '1',


@dzearing apparently this is a bug where providing the type number automatically converts the value into a string with 'px' appended.

Yup, there was a PR to fix it by Micheal but had to be reverted as some people are using lineHeight: 20 intending for it to output line-height: 20px.

dzearing · 2018-02-07T06:20:15Z

im curious if there are any significant performance changes in details list with this change. Check is a high traffic component there.

jordandrako · 2018-02-08T20:01:31Z

It seems the performance impact is minimal, but not sure if these performance tests in dev tools are sufficient.

jordandrako · 2018-02-09T01:43:50Z

@dzearing So I rendered like 5000 Checks with sass and with mergeStyles, and couldn't see any noticeable difference in page load speed or anything. I think perf is fine.

dzearing · 2018-02-09T05:39:54Z

@jordandrako woot, thanks!

dzearing · 2018-02-10T01:13:59Z

packages/office-ui-fabric-react/src/components/Check/Check.styles.ts

+           * TODO: Come back to this once .checkHost has been
+           * converted to mergeStyles
+           */
+          '.checkHost:hover &, .checkHost:focus &, &:hover, &:focus': {


Hmmm. I'm not sure this will work. Is "checkHost" a global? Where is this coming from?

checkHost is coming from the Check.scss file but only used in DetailsRowCheck.tsx.

<div { ...buttonProps } role='checkbox' className={ css(className, 'ms-DetailsRow-check', DetailsRowCheckStyles.check, CheckStyles.checkHost, { [`ms-DetailsRow-check--isDisabled ${DetailsRowCheckStyles.isDisabled}`]: !props.canSelect, [`ms-DetailsRow-check--isHeader ${DetailsRowCheckStyles.isHeader}`]: props.isHeader }) } aria-checked={ isPressed } data-selection-toggle={ true } data-automationid='DetailsRowCheck' > <Check checked={ isPressed } /> </div>

dzearing · 2018-02-10T01:15:46Z

packages/office-ui-fabric-react/src/components/Check/Check.base.tsx

+      <div
+        className={ classNames.root }
+      >
+        { Icon({


I'm confused why this isn't JSX.

<Icon className={ ... } iconName={ ... } />

That was how it was written before, I was trying to keep the number of logic changes unrelated to mergeStyles to a minimum.

dzearing · 2018-02-10T01:16:20Z

packages/office-ui-fabric-react/src/components/Check/Check.styles.ts

@@ -0,0 +1,145 @@
+import { ICheckStyleProps, ICheckStyles } from './Check.types';
+import {
+  concatStyleSets,


Some of these imports aren't needed. Not sure why no-unused-imports isn't picking this up.

Hmm, me either. I'll take the unused ones out.

dzearing · 2018-02-10T01:18:13Z

packages/office-ui-fabric-react/src/components/Check/index.ts

export CheckBase as well perhaps.

Also did you want to delete the scss file?

Alright.

Not yet, there are some rogue components importing it directly. I'll come back to delete once those are converted (namely DetailsList... lol).

dzearing

minor feedback.

…into mergeStyles/Check

Addressed the issue

jordandrako · 2018-02-21T22:28:46Z

@dzearing @battletoilet Could I get approval on this?

dzearing · 2018-02-22T06:49:49Z

I've approved, @jordandrako sorry it got buried in too many PRs. There is a conflict. Can you double check what changed?

dzearing

fix conflicts.

* master: (28 commits) Scrollable pane sort stickies (microsoft#4111) Allow ScrollablePane to accept native properties. (microsoft#4095) Sticky (microsoft#4091) Applying package updates. [ColorRectangle, Sticky] Fixed null root refs (microsoft#4099) DatePicker: order of callbacks for onSelectDate and onAfterMenuDismiss (microsoft#4092) Applying package updates. Alhenry fix split button props (microsoft#4090) Fixing ComboBox styling by reverting button classname move (microsoft#4088) Update CODEOWNERS Undoing terrible change. [DetailsList] Fixed focus test (microsoft#4087) Added icons package screener test (microsoft#4082) ContextualMenu: Fix ContextualMenuUtility imports (microsoft#4085) [ContextualMenu] Made disabled buttons focusable (microsoft#4074) Convert Check to mergeStyles (microsoft#3880) [DetailsList] Add public focusIndex function (microsoft#3852) ComboBox button should have data-is-focusable="false" (microsoft#4070) Applying package updates. Focus Zone: Allow Tab to Skip Selection (microsoft#4061) ...

* Separate types to types.ts file. Start converting check to mergeStyles * Convert bulk scss to mergeStyles. Needs to be reviewed, this is rough. * Add comments for style types. * Convert Check to mergeStyle pattern * Add more props and types * Fix most style conversions. * Remove setUserSelect related lines. Rename rootIsChecked to just checked state. * Remove setUserSelect related lines. Rename rootIsChecked to just checked state. * Fix line-height * Export types in index. * npm run change output * Fix import for Travis * Move className logic to styles file. * nit, remove _ in _sharedCircleCheck * Combine checked conditionals into array. * Make ICheckStyles required, remove checkHost * Export base component * Remove unused import * Make icons jsx * Remove uneeded const

Jordan Janzen added 11 commits February 1, 2018 17:46

Separate types to types.ts file.

6a250b7

Start converting check to mergeStyles

Convert bulk scss to mergeStyles. Needs to be reviewed, this is rough.

b35ac65

Add comments for style types.

fe0a2cc

Convert Check to mergeStyle pattern

077aec6

Add more props and types

653e516

Fix most style conversions.

7043de2

Remove setUserSelect related lines. Rename rootIsChecked to just chec…

90edfb0

…ked state.

Remove setUserSelect related lines. Rename rootIsChecked to just chec…

45ccb3a

…ked state.

merge

b0ae601

Fix line-height

1726d5b

Export types in index.

456a308

jordandrako added the merge-styles label Feb 3, 2018

jordandrako requested a review from dzearing February 3, 2018 01:48

Jordan Janzen and others added 3 commits February 2, 2018 17:49

npm run change output

047428e

Merge branch 'master' into mergeStyles/Check

b3ac3fa

Fix import for Travis

98377dc

dzearing reviewed Feb 6, 2018

View reviewed changes

gokunymbus previously requested changes Feb 6, 2018

View reviewed changes

Jordan Janzen added 2 commits February 6, 2018 15:04

Move className logic to styles file.

5a03aa5

nit, remove _ in _sharedCircleCheck

21832aa

dzearing closed this Feb 7, 2018

dzearing reopened this Feb 7, 2018

Jordan Janzen added 2 commits February 7, 2018 10:55

Merge branch 'master' into mergeStyles/Check

e68c604

Merge branch 'master' into mergeStyles/Check

def08c6

Combine checked conditionals into array.

3872a95

Make ICheckStyles required, remove checkHost

d0d9590

dzearing reviewed Feb 10, 2018

View reviewed changes

dzearing previously requested changes Feb 10, 2018

View reviewed changes

Jordan Janzen added 7 commits February 12, 2018 10:27

Export base component

babf3ba

Remove unused import

04a146c

Make icons jsx

e531bbc

Remove uneeded const

6dd8473

Merge branch 'master' into mergeStyles/Check

d8b8504

Merge branch 'master' into mergeStyles/Check

1d6604c

Merge branch 'master' of github.com:OfficeDev/office-ui-fabric-react …

e0ed9c0

…into mergeStyles/Check

Merge branch 'master' into mergeStyles/Check

a04339d

dzearing approved these changes Feb 22, 2018

View reviewed changes

dzearing self-assigned this Feb 22, 2018

Merge branch 'master' into mergeStyles/Check

4920468

jordandrako merged commit b2177a5 into microsoft:master Feb 23, 2018

micahgodbolt added this to the Convert all components to css-in-js milestone Jul 2, 2018

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Check to mergeStyles #3880

Convert Check to mergeStyles #3880

jordandrako commented Feb 3, 2018 •

edited

Loading

dzearing Feb 6, 2018

gokunymbus Feb 6, 2018

jordandrako Feb 7, 2018

gokunymbus Feb 6, 2018

jordandrako Feb 7, 2018

jordandrako Feb 12, 2018

gokunymbus Feb 6, 2018

jordandrako Feb 7, 2018

dzearing commented Feb 7, 2018

jordandrako commented Feb 8, 2018

jordandrako commented Feb 9, 2018 •

edited

Loading

dzearing commented Feb 9, 2018

dzearing Feb 10, 2018

jordandrako Feb 12, 2018

dzearing Feb 10, 2018

jordandrako Feb 12, 2018

dzearing Feb 10, 2018

jordandrako Feb 12, 2018

dzearing Feb 10, 2018

jordandrako Feb 12, 2018

dzearing left a comment

jordandrako commented Feb 21, 2018

dzearing commented Feb 22, 2018

dzearing left a comment


		const { palette, semanticColors } = theme;

		const _sharedCircleCheck: IStyle = {

Convert Check to mergeStyles #3880

Convert Check to mergeStyles #3880

Conversation

jordandrako commented Feb 3, 2018 • edited Loading

Pull request checklist

Description of changes

Focus areas to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzearing commented Feb 7, 2018

jordandrako commented Feb 8, 2018

jordandrako commented Feb 9, 2018 • edited Loading

dzearing commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzearing left a comment

Choose a reason for hiding this comment

jordandrako commented Feb 21, 2018

dzearing commented Feb 22, 2018

dzearing left a comment

Choose a reason for hiding this comment

jordandrako commented Feb 3, 2018 •

edited

Loading

jordandrako commented Feb 9, 2018 •

edited

Loading