-
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(datepicker): add mdDatepickerToggle & compatibility w/ md-input-container #3468
Conversation
if (this._mdInputContainer) { | ||
return this._mdInputContainer.getPopupConnectionElementRef(); | ||
} else { | ||
return this._elementRef; |
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.
Nit: ternary would be more readable?
<p> | ||
<md-input-container> | ||
<input mdInput [mdDatepicker]="dp2" [(ngModel)]="date"> | ||
<md-datepicker-trigger mdSuffix [for]="dp2"></md-datepicker-trigger> |
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'm guessing you've already thought this through, but any way to avoid requiring this element? If at all possible, it feels like this should be built in somewhere. Can we have the [mdDatepicker]
directive add the class and the open method?
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.
This allows for more flexibility, the icon trigger can be in the prefix, suffix, or completely outside of the input-container.
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.
Gotcha; that makes sense. If it's going to be separate, how about making the datepicker-trigger a button? I think part of the weirdness for me is that the input should itself be a trigger, so adding something else called "trigger" threw me off. Also better to have a native button for a11y.
Jeremy and I were thinking maybe:
<input [mdDatepicker]="dp">
<button mdDatepickerToggle></button>
<md-datepicker #dp></md-datepicker>
src/lib/input/input-container.ts
Outdated
@@ -379,4 +381,8 @@ export class MdInputContainer implements AfterContentInit { | |||
|
|||
this._mdInputChild.ariaDescribedby = ids.join(' '); | |||
} | |||
|
|||
getPopupConnectionElementRef(): ElementRef { |
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 feels like this should be named for the underline, given that there may be other non-popup components that might want to hook in here.
comments addressed |
renamed trigger, PTAL |
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.
LGTM
…ut-container (#3468) feat(datepicker): add mdDatepickerToggle & compatibility w/ md-input-container
…ut-container (angular#3468) feat(datepicker): add mdDatepickerToggle & compatibility w/ md-input-container
…ut-container (#3468) feat(datepicker): add mdDatepickerToggle & compatibility w/ md-input-container
…ut-container (#3468) feat(datepicker): add mdDatepickerToggle & compatibility w/ md-input-container
…ut-container (#3468) feat(datepicker): add mdDatepickerToggle & compatibility w/ md-input-container
…ut-container (#3468) feat(datepicker): add mdDatepickerToggle & compatibility w/ md-input-container
…ut-container (#3468) feat(datepicker): add mdDatepickerToggle & compatibility w/ md-input-container
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.