Skip to content
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: perf problem of Action flyout menu #4474

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

yeze322
Copy link
Contributor

@yeze322 yeze322 commented Oct 22, 2020

Description

#minor

Problem When moving cursor around Action flyout menu, whole UI stuck.

Root cause
According to this issue microsoft/fluentui#5642 , when loading FluentUI's ContextualMenu, browser's focus state will be affected unexpectedly. In our case, when using the Action flyout menu, the browser focus state (triggered by onFocus and onBlur event) would be changed repeatedly for multiple (around 20) times.

Unfortunately, the browser's focus state was saved as a React state in DesignPage and post to Electron via IPC to enable/disbale the Electron app menu. As a result, everytime user moves cursor around the flyout menu, the design page will be redraw for tons of times which led to the UI stuck.

Solution
This PR fixes this issue by saving the focus state as a ref by using useRef() hook rather than useState().
Instead of subscribing the focus state change with useEffect(), whenever the ref is updated, now Composer will proactively report status to Electron app menu.

With this fix, DesignPage won't be redraw when the browser focus state changes.

Task Item

Screenshots

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 55.339% when pulling d33c5f5 on yeze322:electron/flyout-perf into e8d0d55 on microsoft:main.

@cwhitten cwhitten merged commit 31819e7 into microsoft:main Oct 22, 2020
@cwhitten cwhitten mentioned this pull request Nov 13, 2020
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants