-
Notifications
You must be signed in to change notification settings - Fork 285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: enable the input control when selectedPeople is cleared via code. #2792
Conversation
Handle disabling the input when setting the selectedPeople manually Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
…mode Signed-off-by: Martin Musale <martinmusale@microsoft.com>
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
📖 The updated storybook is available here |
1 similar comment
📖 The updated storybook is available here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we need to revised how we're setting disabled throughout this component and ensure that we're making use of reactive properties/state. Perhaps we have a separate @State textInputDisabled property of the class that is used to control the enabled/disabled state of the fluent-text-field.
The current approach feels wrong to me
packages/mgt-components/src/components/mgt-people-picker/mgt-people-picker.ts
Show resolved
Hide resolved
@gavinbarron I agree with the revision. There are two use cases for disabling a picker: when you want it disabled because say you've selected people already and don't want to add others and when you have single selection mode. For the first scenario, passing |
Thank you for reminding me of the limitation around the placeholder text. That ensures that we do need to use a DOM manipulation on the input inside the shadow DOM of the fluent-text-field. I think we should take the opportunity to clean this up a bit though, we have two places where we enable and two places where we disable the input via dom manipulations. I'd like us to have one method that we can call to enable the text box and one to call to disable it. e.g.
This way the code triggering the DOM manipulation doesn't need to know anything about the input it's working with, just to try and take a logical action. |
🚀 New react-contoso sample application deployed here |
1 similar comment
🚀 New react-contoso sample application deployed here |
📖 The updated storybook is available here |
1 similar comment
📖 The updated storybook is available here |
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
@gavinbarron This is fine. In some instances, within these enable/disable scenarios, we've other actions like focus and setting values to empty applied. I think these are blanket actions without effect on a default behavior of a function standpoint. |
🚀 New react-contoso sample application deployed here |
📖 The updated storybook is available here |
packages/mgt-components/src/components/mgt-people-picker/mgt-people-picker.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
🚀 New react-contoso sample application deployed here |
📖 The updated storybook is available here |
packages/mgt-components/src/components/mgt-people-picker/mgt-people-picker.ts
Outdated
Show resolved
Hide resolved
🚀 New react-contoso sample application deployed here |
📖 The updated storybook is available here |
Signed-off-by: Martin Musale <martinmusale@microsoft.com>
🚀 New react-contoso sample application deployed here |
|
📖 The updated storybook is available here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff!
Closes #2683
PR Type
Description of the changes
selectionChanged
custom event in theselectedPeople
setterPR checklist
yarn build
) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)yarn setLicense
)Other information