Expose showRemoveButton on ExtendedPeoplePicker #7904
Conversation
Adjust types to expose the SuggestionItem X button to the ExtendedPicker. Parameterize the extended people picker to accurately reflect its usage + the type constraints on it.
Adjective-Object
changed the title
Adjective-Object
changed the title
|
Size Auditor did not detect a change in bundle size for any component! |
|
Not sure if major changes are encouraged at this time? I thought all major changes were batched, hence why we're on 6.1XX @natalieethell can you comment on if this should be a major change? |
|
@amyngu @Adjective-Object yes, usually if we're making major changes like this to a props interface, we will mark the old prop as deprecated in the comments. Like this: We'll support use of either prop until we remove the old one in a major version bump. |
|
@joschect correct me if I'm wrong on the above! |
|
I think we could get away with setting the default for the type parameter to Any, since that's effectively what it was before if we wanted to be backward compatible |
There was a problem hiding this comment.
I spoke with @KevinTCoughlin and we think this is a good change, but it may be best to hold off on it until a major version bump. @Adjective-Object re: your idea to set the default type to any, @JasonGore I think we could use your input here since you've been doing a lot of work with generics. What do you think?
|
(To be clear, we'd want to get rid of the 'any' at the next major version bump) |
packages/office-ui-fabric-react/src/components/FloatingPicker/BaseFloatingPicker.types.ts
Show resolved
Hide resolved
| */ | ||
| pickerSuggestionsProps?: IBaseFloatingPickerSuggestionProps; | ||
| pickerSuggestionsProps?: IBaseFloatingPickerSuggestionProps<T | any>; |
There was a problem hiding this comment.
Tied to my other comment, if we don't need to expose this type then this wouldn't actually be a breaking API change.
| @@ -86,6 +86,8 @@ export interface IBaseFloatingPickerProps<T> extends React.ClassAttributes<any> | |||
| className?: string; | |||
| /** | |||
| * The properties that will get passed to the Suggestions component. | |||
| * | |||
| * TODO (adjective-object) remove the '| any' before the next major version | |||
There was a problem hiding this comment.
Is this comment still relevant?
There was a problem hiding this comment.
hmm, thought I cleaned this up but I guess I forgot to push
|
One thing to note is that with this change, |
|
Hello @natalieethell! Because this pull request has the |
|
🎉 Handy links: |
Pull request checklist
$ npm run changeDescription of changes
Adjust types to expose the SuggestionItem X button to the
ExtendedPicker. Parameterize the extended people picker to
accurately reflect its usage + the type constraints on it.
Not sure if this should be a breaking change -- an API consumer will need to update their code iff they directly reference the relevant type. I've seen different libraries treat type incompatibilities w/o functional changes as both major and minor changes.
Focus areas to test
ExtendedPeoplePicker demo
Microsoft Reviewers: Open in CodeFlow