Skip to content
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

refactor(datepicker): move 'outsideDays' generation to the service #2421

Conversation

maxokorokov
Copy link
Member

@maxokorokov maxokorokov commented May 29, 2018

The last in series of moving logic from components to the service and utility functions

@maxokorokov maxokorokov added this to the 2.1.0 milestone May 29, 2018
@maxokorokov maxokorokov force-pushed the datepicker/refactor/outside-days-to-service branch from ab39e59 to 60db3bd Compare May 31, 2018 03:35
Copy link
Member

@benouat benouat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me.
I just left one inline comment regarding a removal.

@@ -48,14 +48,14 @@ import {DayTemplateContext} from './datepicker-day-template-context';
</div>
</div>
<ng-template ngFor let-week [ngForOf]="month.weeks">
<div *ngIf="!isCollapsed(week)" class="ngb-dp-week" role="row">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the role="row" here ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate rebase, thanks for the catch!

@pkozlowski-opensource pkozlowski-opensource modified the milestones: 2.1.0, 2.2.0 Jun 1, 2018
@maxokorokov maxokorokov force-pushed the datepicker/refactor/outside-days-to-service branch from 60db3bd to 6718927 Compare June 4, 2018 06:39
@maxokorokov maxokorokov modified the milestones: 2.2.0, 2.1.0 Jun 4, 2018
@maxokorokov maxokorokov merged commit 26b8f3b into ng-bootstrap:master Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants