TextField: Convert to JS Styling #5639

JasonGore · 2018-07-19T22:12:22Z

Pull request checklist

Addresses an existing issue: Fixes TextField: Convert to css-in-js #5369
Include a change request file using $ npm run change

Description of changes

Convert TextField to JS styling. Also takes advantage of @cliffkoh's awesome PR #5513 to allow for passing in Label styling as part of TextField's styles prop. A new example TextField.Styled.Example.tsx was added to demonstrate this feature (and ensure it continues to work.)

Focus areas to test

Make sure all class names and styling are represented by output, as well as user styling.

Microsoft Reviewers: Open in CodeFlow

…c-react into textfieldStyles

JasonGore · 2018-07-19T22:23:45Z

packages/office-ui-fabric-react/src/components/TextField/TextField.types.ts

+export interface ITextFieldSubComponentStyles {
+  // TODO: this should be the interface once we're on TS 2.9.2 but otherwise causes errors in 2.8.4
+  // label: IStyleFunctionOrObject<ILabelStyleProps, ILabelStyles>;
+  label: IStyleFunctionOrObject<any, any>;


I've confirmed this isn't an issue with TS 2.9.2. I tried to find another way to avoid this, particularly since it's public, but could not without forcing a bunch of cascading anys and casts all throughout TextField. If anybody sees an easier solution until we can use a newer version of TS, let me know.

@dzearing @kenotron FYI

dzearing · 2018-07-21T20:12:02Z

packages/office-ui-fabric-react/src/components/TextField/MaskedTextField/MaskedTextField.tsx

        onPaste={this._onPaste}
        value={this.state.displayValue}
-        ref={this._resolveRef('_textField')}
+        componentRef={this._resolveRef('_textField')}


can we use React.createRef here?

Since _resolveRef is deprecated, we could replace its usage throughout Fabric, but I'd rather do that in a separate PR.

oengusmacinog-zz

Looks good! Approving but there is one comment.

oengusmacinog-zz · 2018-07-23T18:56:19Z

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

-                  <p
-                    className={css('ms-TextField-errorMessage', AnimationClassNames.slideDownIn20, styles.errorMessage)}
-                  >
-                    <span className={styles.errorText} data-automation-id="error-message">


Does this span no longer need a style set of its own?

I'm not sure if you're saying it can be removed or if you think it was removed. It's still in ITextFieldStyles with default global class name in TextField.styles.ts and applied on line 229 below.

I am asking if it should be missing or not. I am talking about the span tag style styles.errorText, not the p tag style styles.errorMessage which is reapplied on 229

Actually, I'm not seeing it in the original scss do I must be missing something

Ahh I see. Yeah, I didn't see errorText being defined anywhere. The fact that there wasn't an error here is a problem of using the old any styles. :/

Oh is that why there is no error?! Good to know I was racking my brain trying to figure out how that slipped by. In that case, must just be left over from some point when it did have styles.

dzearing · 2018-07-24T20:17:31Z

packages/experiments/src/components/Form/inputs/textInput/FormTextInput.test.tsx

-      expect(field.state.errorMessage).toBeTruthy();
+      // TODO: this test is not working as intended as even nonsense state names pass.
+      //        commented out for now since test isn't executing when named 'xit'
+      // const field = ReactTestUtils.findRenderedComponentWithType(renderedForm, TextField);


ive seen these tests in form tests randomly fail. We will need to evaluate what the right pattern is here once we gain traction.

cliffkoh · 2018-07-24T23:08:33Z

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

  /** Is true when the control has focus. */
-  isFocused?: boolean;
+  isFocused: boolean;



cliffkoh · 2018-07-24T23:10:06Z

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

    const { isFocused } = this.state;
    const errorMessage = this._errorMessage;

+    this._classNames = getClassNames(styles!, {


! [](start = 43, length = 1)

You don't need the ! here for styles

cliffkoh · 2018-07-24T23:13:55Z

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

    const errorMessage = this._errorMessage;

+    this._classNames = getClassNames(styles!, {
+      theme: theme!,


! [](start = 18, length = 1)

Totally understand your usage here but FWIW, I took a different approach to this in Dropdown to minimize use of !

@cliffkoh Maybe you could just describe the approach right here :)

I will look in the other PR; i'm very curious how you could avoid this.

I think @cliffkoh placed the theme prop as an "optional" in the styleprops definition so it pushes that issue down to styles.ts, but there he added a throw if not set. It's a bit harsh to have an optional that's gets thrown if not set :(

I am in the middle of (hopefully) resolving this issue in createComponent...

cliffkoh · 2018-07-24T23:14:36Z

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

+
+    if (label) {
+      return (
+        <Label required={required} htmlFor={this._id} styles={labelStyles}>


required={required} [](start = 15, length = 19)

👍

cliffkoh · 2018-07-24T23:15:43Z

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

            )}
            {multiline ? this._renderTextArea() : this._renderInput()}
-            {(iconClass || iconProps) && <Icon className={css(iconClass, styles.icon)} {...iconProps} />}
+            {(iconClass || iconProps) && <Icon className={css(iconClass, this._classNames.icon)} {...iconProps} />}


iconClass [](start = 62, length = 9)

Should iconClass here be part of the baseStyles? (this was the approach I took in Dropdown)

This is just a total miss on my part. I never intended for css function to still be used here. Thanks

Since the Icon styles function is already exposed through iconProps I don't think there needs to be another entry for it in subComponentStyles.

cliffkoh · 2018-07-24T23:18:43Z

packages/office-ui-fabric-react/src/components/TextField/TextField.tsx

-                    className={css('ms-TextField-errorMessage', AnimationClassNames.slideDownIn20, styles.errorMessage)}
-                  >
-                    <span className={styles.errorText} data-automation-id="error-message">
-                      {errorMessage}


cliffkoh · 2018-07-24T23:21:36Z

packages/office-ui-fabric-react/src/components/TextField/TextField.types.ts

+  // TODO: this should be the interface once we're on TS 2.9.2 but otherwise causes errors in 2.8.4
+  // label: IStyleFunctionOrObject<ILabelStyleProps, ILabelStyles>;
+  label: IStyleFunctionOrObject<any, any>;
+}


Hmm it's actually interesting you made this a dedicated interface. This way the autodoc picks it up - but somewhat ironically the label itself is missing JSDoc :)

cliffkoh · 2018-07-24T23:23:48Z

packages/office-ui-fabric-react/src/components/TextField/examples/TextField.Styled.Example.tsx

+  public render(): JSX.Element {
+    return (
+      <div className="docs-TextFieldExample">
+        <TextField label="Theme Primary Label, Red Top Border (JS):" required={true} styles={getStyles} />


Love this new example but it would be great to have some exposition providing context for what the example is illustrating (I'm not certain this is necessarily obvious)

While I agree that we should have more on styling, I don't know if this is the right place. We should really have a styling page. I've long envisioned a page where every component is JS styled with multiple themes and customizers on one page that would also serve as a great snapshot test.

I'm actually just suggesting a 1-liner exposition - not a detailed documentation. Completely agree with you this is not the right place.

Yes just an explanation paragraph would be useful.

Many of the examples we provide read like undocumented test cases. It's one of the common complaints we hear. Walk the user through what you're illustrating. We can leverage your example here to repeat it elsewhere.

cliffkoh · 2018-07-24T23:25:03Z

packages/office-ui-fabric-react/src/components/TextField/TextField.test.tsx

+  beforeAll(() => {
+    // Prevent warn deprecations from failing test
+    jest.spyOn(WarnUtil, 'warnDeprecations').mockImplementation(() => {
+      /** no impl **/


If there's a test using deprecated props, shouldn't it be in a .deprecated.test file?

cliffkoh · 2018-07-24T23:27:10Z

packages/office-ui-fabric-react/src/components/TextField/TextField.test.tsx

  it('should associate the label and input box', () => {
-    const renderedDOM: HTMLElement = renderIntoDocument(<TextField label="text-field-label" value="whatever value" />);
+    const renderedDOM: HTMLElement = renderIntoDocument(
+      <TextFieldBase label="text-field-label" value="whatever value" />


TextFieldBase [](start = 7, length = 13)

Out of curiosity, what determines why some tests use TextFieldBase and others use TextField?

Mostly because HOC styled wrapper breaks a lot of these fragile tests. I'll try and get back to making this more consistent.

If it is with enzyme, you can use dive() to get pass the HOC..

No, they're mostly using ReactTestUtils.

cliffkoh · 2018-07-24T23:27:36Z

packages/office-ui-fabric-react/src/components/TextField/TextField.styles.tsx

+    hasErrorMessage
+  } = props;
+
+  const { semanticColors, palette } = theme;


cliffkoh · 2018-07-24T23:32:46Z

packages/office-ui-fabric-react/src/components/TextField/TextField.styles.tsx

+            color: semanticColors.inputPlaceholderText,
+            opacity: 1
+          },
+          ':-ms-input-placeholder': {


':-ms-input-placeholder [](start = 10, length = 23)

I would actually have expected ::placeholder to include all browser variants (this and others like ::-webkit-input-placeholder) automatically hmm, and if so, this might be redundant..

cliffkoh

👍 Typical high quality work.

dzearing · 2018-07-25T14:09:28Z

packages/office-ui-fabric-react/src/components/TextField/TextField.types.ts

 }
+
+export type ITextFieldStyleProps = Required<Pick<ITextFieldProps, 'theme'>> &
+  Pick<


wonder how these props show up in documentation.

My guess is that it's ignored.

Yeah, they don't show up, good catch. Do you think the parser can be modified to pick these up?

I fully expected it to not show up but I would rather optimize for doing the right thing here and improve the parser as a systemic fix. But at the same time, I don't want to unnecessarily invest in the parser atm given API extractor and TSDoc especially given all the other burning problems we have :)

dzearing · 2018-07-25T14:11:52Z

packages/office-ui-fabric-react/src/components/TextField/examples/TextField.Styled.Example.tsx

+import * as React from 'react';
+import { ITextFieldStyleProps, ITextFieldStyles, TextField } from 'office-ui-fabric-react/lib/TextField';
+import { ILabelStyles, ILabelStyleProps } from 'office-ui-fabric-react/lib/Label';
+import './TextField.Examples.scss';


do we need these sass files? If you add a new example, ideally we wouldn't depend on sass. (If you do need them, then it's ok to not change this if it means less changes.)

The new example explicitly has JS and CSS styling, so this is needed for the CSS styling example.

dzearing · 2018-07-25T14:35:04Z

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

                    className={classNames.input}
                    value={color.hex}
-                    ref={ref => (this.hexText = ref!)}
+                    componentRef={this._hexText}


yay thank you!

dzearing · 2018-07-25T14:35:47Z

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


  private _onHexChanged = (): void => {
-    this._updateColor(getColorFromString('#' + this.hexText.value));
+    this._updateColor(getColorFromString('#' + this._hexText.value));


I believe value is deprecated. Use current.

(We REALLY need to enable the deprecations tslint rule.)

This is all pre-existing code. The only reason this changed at all is because TextField is now in an HOC wrapper. I'd rather not make ref-related or deprecation changes to this PR as it's already pretty wide in scope.

dzearing

Just minor comments.

micahgodbolt and others added 16 commits June 6, 2018 12:16

init

4f4176b

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

db1b1e5

…c-react into textfieldStyles

Fix typing errors.

5cbb1ad

Merge branch 'master' into textfieldStyles-jg

0aa285a

Fix typing errors.

02ba7c7

Add more snapshot tests with results from master. Fix other unit tests.

c4d2a81

Cleanup. Temp work added for collaboration with mergeStyles changes.

de36bc5

Revert files to master for clean merge.

bdf4197

Merge branch 'master' into textfieldStyles-jg

275d476

Merge branch 'master' into textfieldStyles-jg

1d7cd5d

Merge branch 'master' into textfieldStyles-jg

d2fffc5

Two-way audit between CSS and JS, typing improvements, styling fixes.

8b9a4cc

Update tests and snapshots, cleanup.

bf03d88

Revert unassociated files to master.

3909760

Revert unassociated files to master.

93b7982

Final cleanup.

ce9ac44

JasonGore requested review from cliffkoh, dzearing, kenotron, kkjeer, micahgodbolt and oengusmacinog-zz July 19, 2018 22:12

JasonGore requested a review from lorejoh12 as a code owner July 19, 2018 22:12

Change files.

83652e2

JasonGore commented Jul 19, 2018

View reviewed changes

JasonGore added 2 commits July 19, 2018 15:56

Comment out type error from unused test.

115dd4e

Change file.

75143d4

dzearing reviewed Jul 21, 2018

View reviewed changes

oengusmacinog-zz approved these changes Jul 23, 2018

View reviewed changes

dzearing reviewed Jul 24, 2018

View reviewed changes

cliffkoh reviewed Jul 24, 2018

View reviewed changes

cliffkoh approved these changes Jul 24, 2018

View reviewed changes

dzearing reviewed Jul 25, 2018

View reviewed changes

dzearing approved these changes Jul 25, 2018

View reviewed changes

JasonGore added the Status: Do Not Merge 🚫 label Jul 25, 2018

JasonGore added 2 commits July 25, 2018 11:22

Address PR feedback.

14f8901

Merge branch 'master' into textfieldStyles-jg

aa7f1fe

JasonGore added Component: TextField and removed Status: Do Not Merge 🚫 labels Jul 25, 2018

JasonGore merged commit d056d71 into microsoft:master Jul 25, 2018

This was referenced Aug 17, 2018

TextField prop inputClassName does not work #5970

Closed

TextField: Fix inputClassName #5974

Merged

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

TextField: Convert to JS Styling #5639

TextField: Convert to JS Styling #5639

Uh oh!

Conversation

JasonGore commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

Uh oh!

JasonGore Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oengusmacinog-zz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzearing Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore commented Jul 19, 2018 •

edited

Loading

JasonGore Jul 19, 2018 •

edited

Loading

JasonGore Jul 23, 2018 •

edited

Loading

JasonGore Jul 23, 2018 •

edited

Loading

dzearing Jul 25, 2018 •

edited

Loading

JasonGore Jul 25, 2018 •

edited

Loading

JasonGore Jul 25, 2018 •

edited

Loading