-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
feat(select): add mat-select-header component #7835
Conversation
118b730
to
9a102e6
Compare
9a102e6
to
1c41bfb
Compare
@crisbeto I am still seeing an issue I mentioned on previous PR;
Happy to open this as a separate issue if it is not in scope for this PR As a slight extension, the following is also true:
|
@crisbeto can we expect it in 2.0.0-beta.13? |
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.
Also update select.md
? In particular I want to mention that the a11y is in the users hand with whatever they put in there.
src/lib/select/select.ts
Outdated
@@ -638,6 +644,10 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit, | |||
if (this.panelOpen) { | |||
this._scrollTop = 0; | |||
this.onOpen.emit(); | |||
|
|||
if (this.header) { | |||
this.header._trapFocus(); |
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.
Add explanation for what you're doing w/ the focus-trap here?
@@ -0,0 +1,3 @@ | |||
<span cdkTrapFocus> |
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.
Do we always want to focus trap here? What's your reasoning for adding it by default instead of letting the user add it?
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'd imagine that we do, just because the primary use case is having a search field. Also it removes a little bit of boilerplate for the consumer.
src/lib/select/select.ts
Outdated
@@ -948,7 +958,7 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit, | |||
this.empty ? 0 : this._getOptionIndex(this._selectionModel.selected[0])!; | |||
|
|||
selectedOptionOffset += MatOption.countGroupLabelsBeforeOption(selectedOptionOffset, | |||
this.options, this.optionGroups); | |||
this.options, this.optionGroups) + (this.header ? 1 : 0); |
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.
Add a comment?
Adds a `mat-select-header` component, which is a fixed header above the select's options. It allows for the user to project an input to be used for filtering long lists of options. **Note:** This component only handles the positioning, styling, some basic focus management and exposes the panel id for a11y. The functionality is up to the consumer to handle. Fixes angular#2812.
1c41bfb
to
2c3d45a
Compare
Rebased and addressed the feedback @jelbourn, can you take another look? |
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.
Revert fdescribe
to describe
so that pull request doesn't break Travis CI.
@@ -56,7 +64,7 @@ const LETTER_KEY_DEBOUNCE_INTERVAL = 200; | |||
const platform = new Platform(); | |||
|
|||
|
|||
describe('MatSelect', () => { | |||
fdescribe('MatSelect', () => { |
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.
It looks like there's a ban on fdescribe
in tslint.json
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.
Is there a way that I can make this change and resubmit? Or is it necessary to wait for @crisbeto to make the change?
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 haven't addressed it yet because I'm waiting for some more feedback, @jrood.
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.
Ok thanks.
@jelbourn does crisbeto's rebase, per your feedback, look good to you? |
This feature would be really handy, hoping for this to get into a release soon :) |
This can be viewed as essential, in my opinion. Just yesterday I was assisting my team lead to try and make this happen in a custom component but we ran into some roadblocks. I sure hope this gets some further attention and gets assigned to a release sooner than later. |
See also #11806 which addresses the a11y concerns via an overlay. |
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
2 similar comments
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
Even @ngbot is so desperate to have this merged in. |
When this will be merged, It's actually solving a very good problem. |
Will this work with the new native select in #12707 , kind of like a datalist? I'm hoping for more native controls to improve the experience on mobile devices (especially some older ones that I have to deal with). |
Will this make it before version 7 is out?... |
Any news about this??? |
have this been forgotten? looks like a merge conflict is all thats keeping i back? |
I think the person that was working on those accessibility things is not working in this team anymore. |
This is a must-have feature at least for any back-end |
the underlying select API is being changed so this is presumably dead |
When this will become available? |
I think it is dead :( |
Any updates? Really disappointed that this important feature is still not exist. |
Any update on this? Waiting desperately for this feature. |
@shreeramk yes, you can find the update in #5697 (comment). |
Closing this since we're going to work this feature into a larger revamp of the select rather than adding it to the current |
Adds a
mat-select-header
component, which is a fixed header above the select's options. It allows for the user to project an input to be used for filtering long lists of options.This is a continuation of #3211 that polishes everything up, sorts out the a11y issues, fixes some animation issues and gets everything working with our current setup.
Note: This component only handles the positioning, styling, some basic focus management and exposes the panel id for a11y. The functionality is up to the consumer to handle.
Fixes #2812. Fixes #5697.