-
Notifications
You must be signed in to change notification settings - Fork 60
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 VideoEffects Pane reseting focus when an item is selected #4618
Conversation
Calling bundle size is increased❗.
|
Chat bundle size is increased❗.
|
CallWithChat bundle size is increased❗.
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
@azure/communication-react jest test coverage for stable.
|
@azure/communication-react jest test coverage for beta.
|
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.
Hmm some of these looking a little flakey potentially
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.
Ah they're not! Every other test performs a click on a specific tile, hence why the focus rect does not show. In this test it does not perform a subsequent click and hence the focus rect shows.
*/ | ||
focusOnMount?: boolean; | ||
componentRef?: React.RefObject<{ |
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.
Interesting so we can set a action to take when the component reference is set?
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.
It's the concept of an imperative handle. You can do this today with regular html elements, e.g. document.getElementById("myTextField").focus();
(ref: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus).
In that example we get the handle by doing document.getElementById
. In react world we can't do that, we have to do one render pass that renders the component and updates the ref, then once its been rendered (i.e. added to the DOM), we can define functions that can be executed on it. The action won't happen when the component reference is set, it happen anytime after its set when the owner of the handle calls the function
What
Instead of trying to change focus on mount, instead use an imperative handle to call
focus()
when openPane is called. We can't use focus on mount, because if the pane is already open its already mounted, but we still need to shift focus.Why
Fix bug where whenever an item was selected, the focus would shift back to the
none
video effects item.How Tested
Locally verified in both configuration page and call page that when the video effects pane is first opened the focus shifts to the first item, confirmed when the pane is already open and the effects button is clicked the focus shifts to the none item, and confirmed when an item is selected the focus does not shift back to the first item.