-
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
fix(datepicker): refactor datepicker toggle to support theming #5317
Conversation
_isButton = false; | ||
|
||
constructor(elementRef: ElementRef, public _intl: MdDatepickerIntl) { | ||
this._isButton = elementRef.nativeElement.tagName.toLowerCase() === 'button'; |
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.
Maybe can be:
elementRef.nativeElement instanceof HTMLButtonElement;
@mmalerba Wondering how does one use Fontawesome icon for md-datepicker-toggle-icon ? |
@yogeshgadge just use a normal |
FYI there's already 9 uses of this inside Google we'd have to update |
<button mdSuffix [mdDatepickerToggle]="minDatePicker"></button> | ||
<button md-icon-button mdSuffix [mdDatepickerToggle]="minDatePicker"> | ||
<md-datepicker-toggle-icon></md-datepicker-toggle-icon> | ||
</button> |
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.
What do you think of just having
<!-- Icon is baked into this -->
<md-datepicker-toggle for="minDatePicker"></md-datepicker-toggle>
If someone wants to have a custom icon, they just do a custom <button>
with the close behavior. [mdDatepickerToggle]="minDatePicker"
is longer than (click)="minDatePicker.toggle()"
, so it seems like there's not much sense in breaking up the toggle behavior and the baked-in icon.
For the baked-in version, all the things someone would typically want to set on the native <button>
are taken care of by the component (content, aria-label, and type).
Changed to suggested API, 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, just needs rebase
Can you try rebasing? Github doesnt say anything but our script does |
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. |
@jelbourn The
md-datepicker-toggle-icon
could easily be replaced with anmd-icon
the only reason it's separate is so users don't need to load a font to use it. Would it be better to have datepicker module register an svg set with just a'material:datepicker:toggle'
icon for now? Other components could create similar sets if they need to include icons.BREAKING CHANGE:
[mdDatepickerToggle]
attribute has been changed tomd-datepicker-toggle
elementfixes #5224
fixes #4934
fixes #4684