FocusZone: restore focus on removing item.#7818
Merged
dzearing merged 29 commits intomicrosoft:masterfrom Jan 31, 2019
dzearing:focuszone
Merged
FocusZone: restore focus on removing item.#7818dzearing merged 29 commits intomicrosoft:masterfrom dzearing:focuszone
dzearing merged 29 commits intomicrosoft:masterfrom
dzearing:focuszone
Conversation
…ric-react into focuszone # Conflicts: # packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.test.tsx # packages/utilities/src/focus.ts
dzearing
commented
Jan 28, 2019
packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.test.tsx
Show resolved
Hide resolved
|
dzearing
commented
Jan 29, 2019
packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.test.tsx
Outdated
Show resolved
Hide resolved
JasonGore
reviewed
Jan 29, 2019
packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| element = isElementTabbable(element) | ||
| ? element | ||
| : getNextElement(this._root.current as HTMLElement, element, true) || getPreviousElement(this._root.current as HTMLElement, element)!; |
Member
There was a problem hiding this comment.
Seeing a ! always makes me wary. Can it actually be undefined here? Other functions that do call this one dereference this function's return value, so it could cause an exception...
packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx
Outdated
Show resolved
Hide resolved
packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx
Outdated
Show resolved
Hide resolved
JasonGore
reviewed
Jan 29, 2019
packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx
Outdated
Show resolved
Hide resolved
dzearing
requested review from
a team,
joschect and
micahgodbolt
as code owners
…ixes to not fire active change events when the active element doesn't change.
joschect
reviewed
Jan 30, 2019
| ? element | ||
| : getNextElement(this._root.current as HTMLElement, element, true) || getPreviousElement(this._root.current as HTMLElement, element)!; | ||
|
|
||
| return element as HTMLElement; |
Contributor
There was a problem hiding this comment.
element should already be an HTMLElement
Member
There was a problem hiding this comment.
It's possibly null. this._root.current! would more clearly communicate that.
| } | ||
| }; | ||
|
|
||
| private _setParkedFocus(isParked: boolean): void { |
Contributor
There was a problem hiding this comment.
Is parked focus just when focus is at the root/highest thing?
Member
Author
There was a problem hiding this comment.
yes, this is a feature Thomas asked for where if you remove the last item in a zone, we put focus on the parent. Then when new focusables are added, we try and restore focus within the zone.
packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx
Outdated
Show resolved
Hide resolved
joschect
approved these changes
Jan 31, 2019
Contributor
|
🎉 Handy links: |
Contributor
|
🎉 Handy links: |
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.
6 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
When focus is lost from the FocusZone due to the item being removed, FocusZone will now move the focus to the item closest to the previously focused element.
Microsoft Reviewers: Open in CodeFlow