fix(datepicker): navigation arrow class override #3922
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3922 +/- ##
==========================================
+ Coverage 91.26% 91.32% +0.06%
==========================================
Files 110 110
Lines 3159 3170 +11
Branches 608 612 +4
==========================================
+ Hits 2883 2895 +12
Misses 195 195
+ Partials 81 80 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hey @ravi9115,
thanks for the PR.
This would be somehow a breaking change for those overriding .right class.
I've opened the #3931 to do what was suggested in the original issue for now.
I still think we should do what you're suggesting in one of the future major releases, but I would also prefer to rename the class to one of the options:
<!-- now -->
<div class="ngb-dp-arrow">
<div class="ngb-dp-arrow right">
<!-- suggestion similar to bootstrap's carousel -->
<div class="ngb-dp-arrow-prev">
<div class="ngb-dp-arrow-next">
<!-- OR similar to buttons -->
<div class="ngb-dp-arrow ngb-dp-arrow-prev">
<div class="ngb-dp-arrow ngb-dp-arrow-next">"Right" doesn't make much sense... especially with future RTL support.
cc @ExFlo
WDYT?
|
Hi @maxokorokov , Thanks for your review. As per your suggestion I've remained the classes similar to bootstrap's carousel. Please review the same. PS: it's my first open source contribution so let me know you have some suggestions on this PR |
There was a problem hiding this comment.
Thanks, @ravi9115!
LGTM apart from forgotten .right, please update.
I'll schedule it for the 10.0.0 when we'll be moving to BS5 and there will be a lot of similar changes.
| @@ -31,7 +31,7 @@ import {NgbDatepickerI18n} from './datepicker-i18n'; | |||
| </div> | |||
| <div class="ngb-dp-arrow" *ngIf="i !== months.length - 1"></div> | |||
| </ng-template> | |||
| <div class="ngb-dp-arrow right"> | |||
| <div class="ngb-dp-arrow-next right"> | |||
There was a problem hiding this comment.
We don't need .right here anymore, do we?
There was a problem hiding this comment.
No, we won't need .right here. I'll remove it
Forgotten during BS5 migration, `right` should become `next`. BREAKING CHANGE: Might be breaking, if arrows are customized. Datepicker navigation arrow markup. BEFORE: ```html <div class="ngb-dp-arrow"> <!-- left --> <div class="ngb-dp-arrow right"> <!-- right --> ``` AFTER: ```html <div class="ngb-dp-arrow ngb-dp-arrow-prev"> <!-- prev --> <div class="ngb-dp-arrow ngb-dp-arrow-next"> <!-- next --> ```
Before submitting a pull request, please make sure you have at least performed the following:
Related issue is: #3872
demo