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(reply): Fix at mention list misaligned after window resize #678

Merged
merged 2 commits into from
Jan 29, 2021

Conversation

mxiao6
Copy link
Contributor

@mxiao6 mxiao6 commented Jan 28, 2021

  • unit test
  • cross-browser testing

@mxiao6 mxiao6 requested a review from a team as a code owner January 28, 2021 23:59
@@ -58,6 +58,10 @@ export default class ReplyField extends React.Component<Props, State> {
fetchCollaborators(trimmedQuery);
}, DEFAULT_COLLAB_DEBOUNCE);

componentDidMount(): void {
window.addEventListener('resize', this.updatePopupReference);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be debounced? Does this conflict with preview's resize handling at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think debounce is needed here since we don't need to catch the new dimensions. We just trigger a popper update, it shouldn't conflict with preview's resize handling.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it looks like calling this.updatePopupReference triggers a setState and also a bunch of logic to get data from the DOM via getVirtualElement. How can we come to further certainty regarding "shouldn't" on the above?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possible conflict that might crop up could be caused by the Preview SDK's own debounced resize handler. Is it possible that the following re-render in the Preview SDK could occur after the resize handler in Annotations? Would that cause the popup to be mispositioned? There may be other examples, such as resize handlers in BUIE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline.

@mxiao6 mxiao6 force-pushed the fix-at-mention branch 2 times, most recently from db59dcb to 6b432bf Compare January 29, 2021 02:01
@mergify mergify bot merged commit 9aca2a1 into box:master Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants