fix(react-combobox): Wrong positioning of the listbox when there's not enough space #31891
+8
−5
Conversation
This file contains 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
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 631 | 631 | 5000 | |
| Button | mount | 297 | 303 | 5000 | |
| Field | mount | 1138 | 1133 | 5000 | |
| FluentProvider | mount | 703 | 707 | 5000 | |
| FluentProviderWithTheme | mount | 87 | 84 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 34 | 37 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 73 | 80 | 10 | |
| MakeStyles | mount | 874 | 884 | 50000 | |
| Persona | mount | 1826 | 1797 | 5000 | |
| SpinButton | mount | 1409 | 1470 | 5000 | |
| SwatchPicker | mount | 1707 | 1729 | 5000 |
2 tasks
📊 Bundle size report
Unchanged fixtures
|
fabricteam
reviewed
Jul 1, 2024
| @@ -1,4 +1,4 @@ | |||
| import { PositioningShorthandValue, resolvePositioningShorthand, usePositioning } from '@fluentui/react-positioning'; | |||
| import { resolvePositioningShorthand, usePositioning } from '@fluentui/react-positioning'; | |||
There was a problem hiding this comment.
🕵 fluentuiv9 Open the Visual Regressions report to inspect the affected screenshots
Avatar Converged 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| Avatar Converged.badgeMask.normal.chromium.png | 5 | Changed |
| Avatar Converged.basic - Dark Mode.normal.chromium.png | 0 | Removed |
dec0943
to
2a90d29
Compare
smhigley
requested changes
Jul 3, 2024
| @@ -10,15 +10,11 @@ export function useComboboxPositioning(props: ComboboxBaseProps): [ | |||
| ] { | |||
| const { positioning } = props; | |||
|
|
|||
| // Set a default set of fallback positions to try if the dropdown does not fit on screen | |||
| const fallbackPositions: PositioningShorthandValue[] = ['above', 'after', 'after-top', 'before', 'before-top']; | |||
There was a problem hiding this comment.
I think changing the fallbackPositions would need to be a larger conversation, since it'd be a breaking change. The current fallbackPositions values matches what we have in v8, which also allows the combobox/dropdown popup to render to the left or right of the trigger.
If we do change this, I think it should include above as a fallback to allow the popup to position above/below based on screen size. In general, I'm a bit leery of this change though, since it could make selection on short screens (and especially with zoom) a lot harder.
Teams can also currently override this through the positioning prop already.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previous Behavior
The listbox is shown from the right or left side when there's no enough space
New Behavior
The listbox is not changing its position
Fixes #29050