Converting existing Toggle component to use Foundation (in experiments) #6959

khmakoto · 2018-11-02T21:11:52Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

Converting existing Toggle component to use Foundation.

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

…c-react into toggle

…oning of tests.

…c-react into toggle

JasonGore · 2018-11-02T21:22:53Z

packages/experiments/src/components/Toggle/Toggle.types.ts

+   * Optional callback to access the IToggleComponent interface. Use this instead of ref for accessing
+   * the public methods and properties of the component.
+   */
+  componentRef?: IRefObject<IToggleComponent>;


BaseComponent manages componentRef for existing styled components. Since Toggle is extending BaseState and not BaseComponent, this currently won't be managed properly.

@dzearing I know we've talked about BaseComponent being bulky, but what do you think about having a newer, more minimal BaseComponent that'll handle componentRef? componentRef seems fairly universal for our components.

I'm thinking BaseState could expand or contain this more minimal BaseComponent functionality.

So, should I just take out componentRef? I did have to change one test because componentRef was working weird, basically checking the same thing without using it.

Originally, the test was:

it('doesn't trigger onSubmit when placed inside a form', () => { let component: any; const onSubmit = sinon.spy(); const wrapper = mount( <form action="#" // tslint:disable-next-line:jsx-no-lambda onSubmit={e => { onSubmit(); e.preventDefault(); }} > <Toggle // tslint:disable-next-line:jsx-no-lambda componentRef={ref => (component = ref)} label="Label" /> </form> ); const button: any = wrapper.find('button'); // simulate to change toggle state button.simulate('click'); expect((component as React.Component<any, any>).state.checked).toEqual(true); expect(onSubmit.called).toEqual(false); });

And I had to change it to:

it('doesn't trigger onSubmit when placed inside a form', () => { const onSubmit = sinon.spy(); const wrapper = mount( <form action="#" // tslint:disable-next-line:jsx-no-lambda onSubmit={e => { onSubmit(); e.preventDefault(); }} > <Toggle // tslint:disable-next-line:jsx-no-lambda label="Label" /> </form> ); const button: any = wrapper.find('button'); // simulate to change toggle state button.simulate('click'); expect( button .first() .getDOMNode() .getAttribute('aria-checked') ).toEqual('true'); expect(onSubmit.called).toEqual(false); });

Just wanted to really double check if it is still doing its intended purpose.

componentRef is supposed to be set to point to the component's instance, but BaseComponent handles that. Right now, nothing is actually setting that value in the new Toggle, so it wouldn't be usable.

I would revert the test and comment it out until we resolve how we deal with componentRef. Also consider temporarily commenting out componentRef prop unless we can resolve it in this PR.

Did you have to change any other tests?

Sounds good, will revert and comment the test for the moment.

Thankfully, this was the only test I had to change.

Unless @dzearing objects, I think we should copy over BaseComponent to where BaseState resides in experiments and minify it just so that it contains the componentRef logic as part of this PR and then have BaseState extend it. I'm still thinking of names (BaseComponentMin? something better?) but once we can promote it we can have the existing BaseComponent extend it.

Also invalid type similar to Button/CS (sorry for not noticing the wrong type before.)

khmakoto · 2018-11-02T21:16:51Z

packages/experiments/src/components/Toggle/Toggle.types.ts

+
+export type IToggleViewProps = Pick<
+  IToggleProps,
+  'as' | 'componentRef' | 'label' | 'onText' | 'offText' | 'ariaLabel' | 'checked' | 'disabled' | 'onChange' | 'keytipProps'


I am not 100% sure if I'm making the proper separation between state and view here. Would like your input on that.

Are these all used by the view? I would only include props actually used by the view.

One consideration is just to have the view simply take in text prop and have state concerned with whether that is onText, offText, or something else. View's props should be very simple and not require much logic, ideally.

As you have it here, some props such as defaultChecked should never present as they are an initializer prop for the state component and do not concern the view. (They do affect checked, but state is responsible for determining the value for view.)

I actually use all of them in Toggle.view.tsx. Well, except for componentRef, which you commented about not working correctly with BaseState.

I think the state prop onText/offText -> view text prop is worthy of changing for simplicity's sake. It's a good demonstration on how view's interface should be really simple while state is concerned with determining view's properties.

This should be done, please take a look at it if you want.

…s state-only props.

JasonGore · 2018-11-02T23:13:43Z

packages/experiments/src/components/Toggle/Toggle.types.ts

+
+export type IToggleViewProps = Pick<
+  IToggleProps,
+  'as' /*| 'componentRef'*/ | 'label' | 'ariaLabel' | 'disabled' | 'onChange' | 'keytipProps'


Actually as I think of this view shouldn't ever need componentRef prop as the state component will have the interface implementation (and will therefore be the actual component reference.)

I removed the prop from the ViewProps list 👍.

JasonGore · 2018-11-02T23:19:26Z

packages/experiments/src/components/Toggle/Toggle.state.ts

+
+    if (!disabled) {
+      // Only update the state if the user hasn't provided it.
+      if (checkedProp === undefined) {


Actually you don't have to do this explicit check. The BaseState constructor takes a second argument which is a list of controlled props. In this case, it should be { controlledProps: ['checked'] }. Button does something similar. This allows you to set checked via setState without fear because BaseState will ensure the proper value is passed to the view.

Although it does raise an interesting question with text.. what if the state's value disagrees with the prop? Then we'll show the wrong text? Hmmm...

It is an interesting question for sure. Should we leave this check? Should we leave the check but only for text and let BaseState handle checked?

I just went ahead and tried deleting the condition and the text was indeed changing although the pill wasn't being toggled.

Another thing we could do is retrieve the state of checked once again after calling this.setState() and use that value to set text. Let me know which one is the option you prefer.

…hanges in text next to toggle.

JasonGore · 2018-11-06T00:21:06Z

packages/experiments/src/utilities/BaseState.tsx

@@ -31,12 +31,19 @@ export class BaseState<TComponentProps, TViewProps, TState> extends React.Compon
    }
  }

+  // Unless overwritten by the component, the props stay the same.
+  public transformDerivedProps(newProps: TViewProps): TViewProps {


Hmm, I think this would be better as part of options along with controlled props.

Also, the typing seems to provide clarity on the name. I wonder if transformViewProps is a better name?

…a part of options along with controlled props.

…nentRef functionality is available in components using Foundation.

khmakoto · 2018-11-06T19:21:30Z

packages/experiments/src/utilities/BaseComponentMin.ts

+ */
+export class BaseComponentMin<TComponentProps extends IBaseProps = {}, TViewProps = {}, TState = {}> extends React.Component<
+  IStateComponentProps<TComponentProps, TViewProps>,
+  TState


@JasonGore I have some doubts about a change I made here to make it work with the separation between state and view but I don't know if it was the correct thing to do.

Originally, this was:

export class BaseComponent<TProps extends IBaseProps = {}, TState = {}> extends React.Component<TProps, TState> {

What do you think?

This is at a lower level so BaseComponentMin should not have to know anything about state components or state vs. view classes, and shouldn't be using anything from IStateComponentProps. The only thing BaseComponentMin should be concerned about is that props has componentRef, which is covered by extends IBaseProps. Line 21 should be able to be just TComponentProps.

JasonGore · 2018-11-06T20:59:21Z

packages/experiments/src/utilities/BaseComponentMin.ts

+
+  // tslint:disable:variable-name
+  private __disposables: IDisposable[] | null;
+  private __resolves: { [name: string]: (ref: React.ReactNode) => React.ReactNode };


Do we need either of these for supporting componentRef?

I think we can safely remove __resolves, but we need __disposables for componentWillUnmount().

JasonGore · 2018-11-06T21:07:12Z

packages/experiments/src/utilities/BaseComponentMin.ts

+      'componentWillReceiveProps',
+      'render',
+      'componentDidUpdate',
+      'componentWillUnmount'


Hmm, this seems to need trimming from original use cases here and in BaseComponent. At least for here, only keep the ones we actually use for now.

…nentRef to missing components in experiments.

JasonGore · 2018-11-06T22:14:11Z

packages/experiments/src/components/Button/Button.types.tsx

@@ -13,6 +13,7 @@ export type IButtonSlots = 'root' | 'stack' | 'text' | 'icon' | 'menuIcon';

 export interface IButtonProps extends IStyleableComponentProps<IButtonProps, IButtonStyles> {
  as?: keyof JSX.IntrinsicElements;
+  componentRef?: IRefObject<IButtonComponent>;


Hmm, this isn't correct, but I can see how the naming is confusing. IButtonComponent should probably be called IButtonComponentTypes (and IComponent be called IComponentTypes) as they are defining types, not actual methods on the class. componentRef must be provided an interface implemented by the component. In this case, Button will eventually need to implement something like IButton but can be {} for now.

Or possibly better yet for each case (changing naming appropriate), define an interface IButton = {} then pass IButton to componentRef.

JasonGore · 2018-11-06T22:15:36Z

packages/experiments/src/components/CollapsibleSection/CollapsibleSection.types.ts

+   * Optional callback to access the ICollapsibleSectionComponent interface. Use this instead of ref for accessing
+   * the public methods and properties of the component.
+   */
+  componentRef?: IRefObject<ICollapsibleSectionComponent>;


Same as button, should be {} for now.

JasonGore

Looks great! Thanks for dealing with the churn

msft-github-bot · 2018-11-08T04:19:19Z

🎉@uifabric/experiments@v6.41.0 has been released which incorporates this pull request.:tada:

Handy Links:

* Add styling api change. * Adding finished Toggle transition to use Foundation. Missing transitioning of tests. * Adding tests for Toggle component using Foundation. * Adding change file. * Deleting unimportant auto-generated comments. * Modifying state test file. * Commenting out 'componentRef' and relegating 'offText' and 'onText' as state-only props. * Removed 'componentRef' from view props. * Adding derived props transformation to BaseState.tsx to account for changes in text next to toggle. * Renaming transformDerivedProps into transformViewProps and making it a part of options along with controlled props. * Using new transformViewProps api in Toggle component. * Creating BaseComponentMin and extending it to BaseState so that componentRef functionality is available in components using Foundation. * Correcting typing errors when using BaseComponentMin and adding componentRef to missing components in experiments. * Using exported interface in componentRef. * Changing componentRef from using IToggleComponent to using IToogle. * Getting rid of disposables.

khmakoto added 8 commits October 31, 2018 15:35

Add styling api change.

44506ac

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

43c4e94

…c-react into toggle

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

1b717de

…c-react into toggle

Adding finished Toggle transition to use Foundation. Missing transiti…

406d7c5

…oning of tests.

Adding tests for Toggle component using Foundation.

1f05f17

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

a352a6a

…c-react into toggle

Adding change file.

3c58451

Deleting unimportant auto-generated comments.

ad45830

khmakoto requested a review from phkuo as a code owner November 2, 2018 21:11

khmakoto requested review from JasonGore and dzearing November 2, 2018 21:12

JasonGore reviewed Nov 2, 2018

View reviewed changes

cliffkoh added the Component: Toggle label Nov 2, 2018

khmakoto commented Nov 2, 2018

View reviewed changes

JasonGore requested a review from kenotron November 2, 2018 21:29

khmakoto added 2 commits November 2, 2018 14:31

Modifying state test file.

ce18bfb

Commenting out 'componentRef' and relegating 'offText' and 'onText' a…

ae87d2e

…s state-only props.

JasonGore reviewed Nov 2, 2018

View reviewed changes

Removed 'componentRef' from view props.

e9bedf0

JasonGore reviewed Nov 2, 2018

View reviewed changes

Adding derived props transformation to BaseState.tsx to account for c…

ba232c6

…hanges in text next to toggle.

JasonGore reviewed Nov 6, 2018

View reviewed changes

khmakoto added 3 commits November 5, 2018 16:44

Renaming transformDerivedProps into transformViewProps and making it …

1f5f31d

…a part of options along with controlled props.

Using new transformViewProps api in Toggle component.

941683d

Creating BaseComponentMin and extending it to BaseState so that compo…

5a60df5

…nentRef functionality is available in components using Foundation.

khmakoto commented Nov 6, 2018

View reviewed changes

JasonGore reviewed Nov 6, 2018

View reviewed changes

Correcting typing errors when using BaseComponentMin and adding compo…

5237caf

…nentRef to missing components in experiments.

JasonGore reviewed Nov 6, 2018

View reviewed changes

khmakoto added 3 commits November 6, 2018 16:20

Using exported interface in componentRef.

397d7d8

Changing componentRef from using IToggleComponent to using IToogle.

fb73d87

Getting rid of disposables.

1a5b0ed

JasonGore approved these changes Nov 7, 2018

View reviewed changes

khmakoto added Component: Toggle and removed Component: Toggle labels Nov 7, 2018

JasonGore merged commit d6be126 into microsoft:master Nov 7, 2018

khmakoto deleted the toggle branch November 8, 2018 19:43

micahgodbolt changed the title ~~Converting existing Toggle component to use Foundation~~ Converting existing Toggle component to use Foundation (in experiments) Nov 14, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converting existing Toggle component to use Foundation (in experiments) #6959

Converting existing Toggle component to use Foundation (in experiments) #6959

khmakoto commented Nov 2, 2018 •

edited by fabricteam

Loading

JasonGore Nov 2, 2018 •

edited

Loading

JasonGore Nov 2, 2018

khmakoto Nov 2, 2018

JasonGore Nov 2, 2018 •

edited

Loading

khmakoto Nov 2, 2018 •

edited

Loading

JasonGore Nov 2, 2018 •

edited

Loading

JasonGore Nov 6, 2018

khmakoto Nov 2, 2018

JasonGore Nov 2, 2018

khmakoto Nov 2, 2018

JasonGore Nov 2, 2018 •

edited

Loading

khmakoto Nov 2, 2018

JasonGore Nov 2, 2018

khmakoto Nov 2, 2018

JasonGore Nov 2, 2018 •

edited

Loading

JasonGore Nov 2, 2018

khmakoto Nov 2, 2018

khmakoto Nov 2, 2018 •

edited

Loading

JasonGore Nov 6, 2018

khmakoto Nov 6, 2018

JasonGore Nov 6, 2018

JasonGore Nov 6, 2018

khmakoto Nov 6, 2018

JasonGore Nov 6, 2018

JasonGore Nov 6, 2018 •

edited

Loading

JasonGore Nov 6, 2018 •

edited

Loading

JasonGore Nov 6, 2018

JasonGore left a comment

msft-github-bot commented Nov 8, 2018

Converting existing Toggle component to use Foundation (in experiments) #6959

Converting existing Toggle component to use Foundation (in experiments) #6959

Conversation

khmakoto commented Nov 2, 2018 • edited by fabricteam Loading

Pull request checklist

Description of changes

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

JasonGore Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonGore Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

khmakoto Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

JasonGore Nov 2, 2018 • edited Loading

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

JasonGore Nov 2, 2018 • edited Loading

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

JasonGore Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khmakoto Nov 2, 2018 • edited Loading

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

JasonGore Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

JasonGore Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonGore left a comment

Choose a reason for hiding this comment

msft-github-bot commented Nov 8, 2018

khmakoto commented Nov 2, 2018 •

edited by fabricteam

Loading

JasonGore Nov 2, 2018 •

edited

Loading

JasonGore Nov 2, 2018 •

edited

Loading

khmakoto Nov 2, 2018 •

edited

Loading

JasonGore Nov 2, 2018 •

edited

Loading

JasonGore Nov 2, 2018 •

edited

Loading

JasonGore Nov 2, 2018 •

edited

Loading

khmakoto Nov 2, 2018 •

edited

Loading

JasonGore Nov 6, 2018 •

edited

Loading

JasonGore Nov 6, 2018 •

edited

Loading