-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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): improve default look and feel #1205
feat(datepicker): improve default look and feel #1205
Conversation
`], | ||
host: { | ||
'[class.bg-primary]': 'selected', | ||
'[class.text-white]': 'selected', | ||
'[class.text-muted]': 'isMuted()', | ||
'[class.outside]': 'isMuted()', |
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 is the reason for the different classes if they go off the same exact check? Is it possible to have text-muted without outside, or vice versa?
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.
text-muted
is the bootstrap class. I wanted to add 50% opacity on top of it, so two options:
[style.opacity]="isMuted() ? 0.5 : 1"
- add the css class
I picked the last one, not sure which one is better though
<div class="ngb-dp-navigation bg-faded"> | ||
<ngb-datepicker-navigation *ngIf="navigation !== 'none'" | ||
[date]="months[0]?.firstDate" | ||
[minDate]="_minDate" |
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.
Should private variables be used in templates? Not sure if these should be public or not. Maybe they are, just going by the name of the var here.
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.
Yeah, this is not a private variable... the thing is there is an @Input() minDate: NgbDateStruct
and an internal data model _minDate: NgbDate
Not sure about this whole NgbDateStruct
vs NgbDate
thing, but it's a whole another story :)
</div> | ||
|
||
<div class="ngb-dp-months d-flex"> | ||
<div class="ngb-dp-month-spacer-sm bg-faded" [style.height.rem]="navigation !== 'select' || displayMonths > 1 ? 4 : 2"></div> |
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 don't mind it that much, but might it be easier to read if you move this logic to a function? Or is the standard to keep simple stuff like this int he HTML?
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 just simplified layout significantly and got rid of spacers altogether. Done with css now. Thanks for pointing this
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 posted a few basic questions which probably don't need changes. Looks good man!
6504273
to
ca410b4
Compare
BREAKING CHANGES: switched to flex layout instead of table-based (drop IE9 support) and prefixed datepicker-related css classes with `ngb-dp-` Fixes ng-bootstrap#706 Fixes ng-bootstrap#1061
ca410b4
to
5eeb8b7
Compare
Amended:
P.S. Tested in IE 10, 11, Edge. Seems to look fine. |
Hi @maxokorokov, thanks for the visual refresh on this. Do you happen to know what version this will land / landed in? I just installed I have this in my package.json: |
Updating default datepicker look and feel as a part of #862 :
ngb-dp-
Before:
After:
P.S. Will also add full month names with a separate request later
BREAKING CHANGES: switched to flex layout instead of table-based (drop IE9 support) and prefixed datepicker-related css classes with
ngb-dp-
Fixes #706