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

feat(datepicker): add activeDate to customClass for custom styling #5812

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jasonhargrove
Copy link

@jasonhargrove jasonhargrove commented Apr 16, 2016

Proposed enhancement that passes the activeDate into the customClass function which decorates the td element.

Currently the active class is attached to the child button. The active date is a prominent feature of the calendar and thus feels important enough in the custom class context to justify this change.

It's also an inexpensive addition that provides a non-invasive opportunity to address issues with the current active class. One example issue is here #3879.

Here's a quick example that doesn't cover all use-cases.

<!-- html directive -->
custom-class="vm.calendar.getDayClass(date, mode, activeDate)"
// controller
vm.calendar.getDayClass = function (date, mode, activeDate) {
  // Custom active class fix
  var activeMonth = new Date(activeDate).getMonth();
  var currentMonth = new Date().getMonth();
  if (activeMonth !== currentMonth) {
    return 'non-current-month';
  }

  // default
  return '';
};

On td.non-current-month button.active, disable the styling.

In this case, the enhancement is used to override a design flaw where there is no today class, and active is attached to today, as well as the 1st of every non current month.

@jasonhargrove jasonhargrove changed the title fix(feat): add activeDate to customClass for custom styling feat(datepicker): add activeDate to customClass for custom styling Apr 16, 2016
@icfantv
Copy link
Contributor

icfantv commented Apr 16, 2016

I could potentially see a use case whereby we keep the currently selected day/month when we navigate forward and backwards through the calendar rather than the current behavior. That said, this should be done in code as there is a specific line that handles this. I'm on mobile right now so I can't easily link it. I will say that at first glance the solution above seems fairly involved for what sounds like a simple change.

All this said, if this is something we decide to pursue, we are going to need tests.

@jasonhargrove
Copy link
Author

jasonhargrove commented Apr 17, 2016

Thanks for your feedback @icfantv . I feel as though the current functionality could be broken out a bit further, for example, today's date should have today class attached to it. While active should be attached to a day the user has clicked.

What I've submitted is the ability for a custom override on what is supported today, without resorting to manipulating the DOM directly. I guess that's the whole point of customClass. If new features were to be released, I would also hope to see those passed into the customClass function for override.

Technically it's a new enhancement that doesn't address #3879 directly, but let's you workaround it, or do other things of your own design. Ideally this would be added separate of fixing the active & today issues, which would be breaking changes.

Sure I can submit some tests, but perhaps we can get a bit more feedback on it first. I may be wrong but having had a quick look, it seemed like there was no existing testing for the customClass feature.

@jasonhargrove
Copy link
Author

Opened another PR with test coverage that addresses the .today issue that inspired this for me. #5814

I still do believe that activeDate should be passed into customClass. Did not find any existing test coverage for the feature.

While working on todayDate, took a look at the activeDate issue and there's a lot of code referencing it. Willing to take a deeper look once there's more consensus for how it should work, or if I find more time in the next week +

@icfantv
Copy link
Contributor

icfantv commented Apr 18, 2016

@jasonhargrove, right now, the biggest hurdle is that the datepicker API is already way too bloated and we are extremely reticent to add any more configuration to it.

all this said, i've just done some quick scouring on the web and none of the jquery, angular material, or bootstrap datepicker implementations have this behavior. if anything, removing the active class functionality entirely seems to be a better UX and more inline with what everyone else is doing.

i'm going to talk to the team and see what we want to do about this.

@jasonhargrove
Copy link
Author

@icfantv it is definitely bloated, and removing it would certainly solve the part of the problem where it's broken. There's a lot of code that seems to depend on this setting though, so removing it sounds like a job in and of itself.

I looked at jQuery datepicker just now and indeed there is no active support, though they do have support for today which is ultimately the thing we really needed; which active seems to be confused with. There's a very simple PR open for that here: #5813

Thanks for taking a deeper look, please let me know if I can assist.

@icfantv
Copy link
Contributor

icfantv commented Jun 24, 2016

@wesleycho, what are your thoughts on this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants