-
Notifications
You must be signed in to change notification settings - Fork 10
Align positioning behavior of rich-text mention listbox, combobox, select, and menu button #2597
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
base: main
Are you sure you want to change the base?
Conversation
packages/nimble-components/src/rich-text/mention-listbox/index.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/rich-text/mention-listbox/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been holding off reviewing this since it looks like there are active discussion threads which I don't have much of an opinion on. Please let me know if you're expecting my input on any of them, otherwise I'll take a look once they're resolved and my vote's reset.
bc92227
to
5d67687
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed stale comment.
name: 'position', | ||
options: [DropdownPosition.above, DropdownPosition.below], | ||
control: { type: 'select' }, | ||
options: [undefined, ...Object.values(DropdownPosition)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We weren't providing a way to demo the default (unset) dropDownPosition
behavior.
name: 'position', | ||
options: ['above', 'below'], | ||
control: { type: 'select' }, | ||
options: [undefined, ...Object.values(DropdownPosition)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like with the combobox stories, we weren't providing a way to demo the default (unset) dropDownPosition
behavior.
} | ||
:host([open][position='below']) .anchored-region { | ||
:host([open]) .anchored-region { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpler and harmless to add padding to both top and bottom of anchored region without worrying about its positioning.
Pull Request
π€¨ Rationale
The mention listbox only opens down, this change allows the mention listbox to open based on available viewport space. Fixes #2246
π©βπ» Implementation
Of the controls that implement anchored region, the select and combobox are most aligned to the rich text mention listbox as they are all Listbox implementations with sized listboxes
This PR updates the richtext mention listbox to align with the select and combobox sizing behavior but does not bring forward the configurable positioning behavior.
dynamic
positioning because it does not have a user configurable positioning like select and combobox.PR also includes:
DropdownPosition
enum consistentlyπ§ͺ Testing
β Checklist