Update ExtendedPeoplePicker component to set the spellCheck and autoC…

[aleury]

Pull request checklist

• Addresses an existing issue: Fixes ExtendedPeoplePicker should pass in autocorrect=off and spellcheck=false to Autofill as native props #6470
• Include a change request file using $ npm run change

Description of changes

In order to set spellCheck to false and autoCorrect to 'off' in the ExtendedPeoplePicker component, I'm overriding a new method, named inputProps, that I added to BaseExtendedPicker.

Test cases are also included to check that these properties are set and are overwritten when the opposite values are passed in by the user (i.e. spellCheck: true and autoCorrect: 'on').

Microsoft Reviewers: Open in CodeFlow

[msftclas]

All CLA requirements met.

[KevinTCoughlin]

@aleury thanks for this contribution and a special thanks for providing test coverage 👏. Adding a few folks with more experience with the Pickers to this review.

[joschect]

I don't think that this is the right way to achieve this. Instead you could do something like

export class ExtendedPeoplePicker extends BaseExtendedPeoplePicker {
  protected _skipComponentRefResolution = true;
  public render() {
    return <BaseExtendedPeoplePicker 
    {...this.props} 
    inputProps={{...this.props.inputProps, ...overwrittenInputProps}}
    />
  }
}

That way you don't need to mess with your BaseExtendedPicker, you don't need to have a method called, and you can use the component ref stuff to ensure you won't lose out on the interface methods you might call.

For example of how some of this works, check out the various buttons. Although I'm not 100% we want to continue that pattern. @dzearing or @micahgodbolt what are your thoughts?

[KevinTCoughlin]

@JasonGore may have some thoughts on how to best override component props from the base class here when he has a moment.

[JasonGore]

The best way (and official React way) is not to use inheritance at all.

In this case, I think the sanest approach is just to do:

class ExtendedPeoplePicker extends React.Component {
    render() {
        return <BaseExtendedPeoplePicker {...this.props} inputProps={{...this.props.inputProps, ...overwrittenInputProps} />;
    }
}

If there is a reason not to use composition, then at the very least @joschect's advice should be taken as it doesn't require the base class to be modified at all and inputProps member can be removed.

[EDIT] Code corrected per my latest comments below.

[JasonGore]

(Changed to extends BaseComponent to make sure refs still work.)

[aleury]

Thanks for the input @joschect and @JasonGore! My initial thought was to go with something similar to what @joschect suggested, but I thought it was a bit weird to render the component that it's also extending. I was aware of the official recommendation of not using component inheritance, but I didn't want to make that assumption for this project and remove it without further input.

I like @JasonGore's suggestion, but am happy to go with @joschect if desired.

[JasonGore]

Oops, I was wrong about my comment. Extending React.Component should be sufficient. Similar to our other HOC wrappers (styled, etc.) the refs will just get forwarded in the props and should continue to work fine.

[joschect]

@aleury Go with Jason's suggestion, it looks more correct.

[yumikohey]

@KevinTCoughlin
I think we should actually have spellCheck and autoCorrect defaults to false for BasePicker, should it?

[KevinTCoughlin]

@KevinTCoughlin
I think we should actually have spellCheck and autoCorrect defaults to false for BasePicker, should it?

Unsure, I defer to component owners here cc: @philipzloh.

[metapzl]

I think we should actually have spellCheck and autoCorrect defaults to false for BasePicker, should it?

As long as we're clear that BasePicker is for "people" / "names", then yes... defaulting to false makes sense

[FrancisMengx]

Looks good after change the function override to the render function.

[Vitalius1]

@KevinTCoughlin we now have 2 PRs that address the same issue #7301. This needs to be figured out by the component owner (cc: @philipzloh, @FrancisMengx ). Let's give it a push and have this issue sorted by the end of this week and not keep 2 PRs open for a fix which seemed to be a straightforward one.

Edit: It's actually the same issue but in different pickers (which raises the question of why we have so many pickers that are so similar in what they do)

[KevinTCoughlin]

@KevinTCoughlin we now have 2 PRs that address the same issue #7301. This needs to be figured out by the component owner (cc: @philipzloh, @FrancisMengx ). Let's give it a push and have this issue sorted by the end of this week and not keep 2 PRs open for a fix which seemed to be a straightforward one.

Edit: It's actually the same issue but in different pickers (which raises the question of why we have so many pickers that are so similar in what they do)

I've spoken offline with the component owners. They believe there is feedback to be addressed above that remains unresolved.

[msft-github-bot]

This pull request has been automatically marked as stale because it has marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. Thank you for your contributions to Fabric React!

[msft-github-bot]

This pull request has been automatically marked as stale because it has marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. Thank you for your contributions to Fabric React!