Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

feat(datepicker): Provide a 'today' flag on the day object #1551

Closed
wants to merge 1 commit into from

Conversation

perry
Copy link

@perry perry commented Jan 9, 2014

This adds a highlight to the button if it matches todays date.

@pkozlowski-opensource
Copy link
Member

@bekos would you mind reviewing this one?

@@ -14,7 +14,7 @@
<tr ng-repeat="row in rows">
<td ng-show="showWeekNumbers" class="text-center"><em>{{ getWeekNumber(row) }}</em></td>
<td ng-repeat="dt in row" class="text-center">
<button type="button" style="width:100%;" class="btn btn-default btn-sm" ng-class="{'btn-info': dt.selected}" ng-click="select(dt.date)" ng-disabled="dt.disabled"><span ng-class="{'text-muted': dt.secondary}">{{dt.label}}</span></button>
<button type="button" style="width:100%;" class="btn btn-default btn-sm" ng-class="{'btn-info': dt.selected, 'today': dt.today}" ng-click="select(dt.date)" ng-disabled="dt.disabled"><span ng-class="{'text-muted': dt.secondary}">{{dt.label}}</span></button>
Copy link
Contributor

Choose a reason for hiding this comment

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

@perry I am not keen of adding dummy CSS classes like today. You have to provide a nice visual hint for the default template also. Anyone can override the default template later.

Choose a reason for hiding this comment

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

It's probably best to use .text-primary or .text-info and not a button class since that would put to much (visual) focus on today, compared to the currently selected day.

Choose a reason for hiding this comment

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

Totally agree with @bekos here. We either need to remove this class from the default template or, better yet, make use of on bootstrap class to highlight "today".

@bekos
Copy link
Contributor

bekos commented Jan 11, 2014

@perry This is a nice feature we would like to add 👍 .

Left some comments on the code, but I would also prefer to support this for every mode, not just the day . On the side, you have to provide accompanying tests, in order to consider merging :-)

@pkozlowski-opensource
Copy link
Member

@perry there are some comments in the PR, could you have a look at those? Also, could you please rebase your PR on top of the current master?

@perry
Copy link
Author

perry commented Jan 21, 2014

Thanks for the feedback everyone. I'll try and work on this as soon as I get the chance.

@bekos
Copy link
Contributor

bekos commented Jan 24, 2014

@perry Thanks for the PR, but the datepicker is under refactor on #1599, so this is difficult to be merged atm.
I am sure your issue will be take into account :-)

@bekos bekos closed this Jan 24, 2014
@henrahmagix henrahmagix deleted the today-highlight branch January 16, 2015 14:32
@henrahmagix henrahmagix restored the today-highlight branch January 16, 2015 14:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants