Callout/Positioning: perf improvement with hidden flag and improved repositioning logic#4419
Merged
joschect merged 14 commits intomicrosoft:masterfrom Apr 4, 2018
Merged
Callout/Positioning: perf improvement with hidden flag and improved repositioning logic#4419joschect merged 14 commits intomicrosoft:masterfrom
joschect merged 14 commits intomicrosoft:masterfrom
Conversation
joschect
requested review from
aditima,
dzearing,
manishgarg1,
mdahamiwal,
micahgodbolt and
montselozanod
dzearing
reviewed
Apr 2, 2018
| */ | ||
| getStyles?: IStyleFunction<ICalloutContentStyleProps, ICalloutContentStyles>; | ||
| /** | ||
| * Whether or not the callout should be hidden rather than displayed. |
Member
There was a problem hiding this comment.
Better text:
If specified, renders the Callout in a hidden state. Use this flag, rather than rendering a callout conditionally based on visibility, to improve rendering performance when it becomes visible.
dzearing
reviewed
Apr 2, 2018
| if (this.props.setInitialFocus && !this._didSetInitialFocus && this.state.positions && this._calloutElement.value) { | ||
| this._didSetInitialFocus = true; | ||
| focusFirstChild(this._calloutElement.value); | ||
| this._async.requestAnimationFrame(() => focusFirstChild(this._calloutElement.value!)); |
Member
There was a problem hiding this comment.
There is a new helper focusAsync, which does this. I wonder if focusFirstChild should just call focusAsync.
Contributor
Author
There was a problem hiding this comment.
That would make a lot of sense, especially since focus triggers reflows.
dzearing
approved these changes
Apr 3, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ npm run changeDescription of changes
Note: This is a first round, comments are welcome
For when perf is a concern, isHidden can now be passed into the callout. If isHidden is true the callout will render, but it's content will not. When isHidden changes from true, to false, the callout will appear and render. This decreases the amount of layout thrashing and significantly reduces the amount of time it takes for the callout to appear. It does, however, increase the amount of dom complexity as the it's layer will be present. isHidden should be used sparingly and only when perf is a real concern.
Focus areas to test
Callout, contextualmenu