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

(WIP) => fix(date-picker): Updates avalible years to be dynamic to min and max #10910

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@raygervais
Contributor

raygervais commented Apr 19, 2018

This fixes #10646.

This removes the years which were originally greyed out. Brings in 4 edge cases:

  1. No min / max: Standard Behavior
  2. Only min: offset to 0 (display min year as first)
  3. Only max: Display all paged years minus 1.
  4. Both min and max: Same as 3.

@raygervais raygervais requested a review from mmalerba as a code owner Apr 19, 2018

@googlebot googlebot added the cla: yes label Apr 19, 2018

@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 19, 2018

I will update with screens and progress, just wanted to show initial thinking based on @WizardPC logic described in the bug and fix any syntax compliance issues.

@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 20, 2018

Default behaviour (Pre Update where no min / max being provided):
screen shot 2018-04-20 at 11 03 10 am

Providing a Min Date:
Will fix heading which is displaying wrong year range
screen shot 2018-04-20 at 11 04 22 am

I'm currently fixing the logic for case 3 and 4, which is broken. Opinions so far?

@mmalerba

This comment has been minimized.

Contributor

mmalerba commented Apr 20, 2018

It seems reasonable to me, couple comments:

  • For the first case, would it make more sense to put today's date somewhere near the middle? That way the user can go forward or back a couple years without changing pages.
  • The header in both images still says 2016-2039. I assume you just haven't gotten around to changing it yet?
@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 20, 2018

Working on your first point now, since I believe that will help lead to the correct design. I'm partially lost how to tackle case 3 & 4. And yes, I have not gotten around to changing the header yet. Any thoughts / suggestions towards the header being change, or case 3 / 4?

@mmalerba

This comment has been minimized.

Contributor

mmalerba commented Apr 20, 2018

I think the math would look something like this for the max year case:

If we were to put the current year at the end of the page, how many pages would we have to flip through to get to the max year? we can determine that like this: currentYear + yearsPerPage * x = maxYear

We want x to come out non-fractional here, because that means that the maxYear will also land at the end of the page. If x turns out fractional we can just move the currentYear back enough so that the maxYear lands at the end, like this: currentYearOffsetFromEnd = (x - floor(x)) * yearsPerPage.

If you want the offset from the beginning instead you can just do currentYearOffsetFromStart = yearsPerPage - 1 - currentYearOffsetFromEnd to get that.

@WizardPC

This comment has been minimized.

WizardPC commented Apr 20, 2018

@raygervais for the header, i think it's wil be the same but in get periodButtonText() from /src/lib/datepicker/calendar.ts.

I will try to find time to do a deeper analysis this week-end... be brave !

@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 20, 2018

Stupid question @mmalerba, perhaps it's been a long week but when you're referring to x, thats a value which we have to determine right? I need to find the best value be it 1, 200, etc.

Here the code logic I interpreted from above, again apologies if this is being overcomplicated or I'm thinking vastly different.

 let x = 1;
 let currentYearOffsetFromEnd = (x - Math.floor(x)) * yearsPerPage;

 let currentYearOffestFromStart = yearsPerPage - 1 - currentYearOffsetFromEnd;

 let maxYear = activeYear + yearsPerPage;
@mmalerba

This comment has been minimized.

Contributor

mmalerba commented Apr 20, 2018

Yeah x is something we need to calculate based on the max and the current year. currentYear + yearsPerPage * x = maxYear, so x = (maxYear - currentYear) / yearsPerPage

@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 21, 2018

Thanks for the clarification @mmalerba, I've updated the codebase to accommodate that the last two edge cases which appears to be working. Will post screenshots soon with the current states so we can discuss / finalize what is best.

Looking into the header now, since I still see 2016-2039 even when using a
minDate: 2/9/2022 and maxDate: 7/16/2031.

Also, I noticed that once the year difference between max and min is greater than 22, the final year is duplicated in a new page. I'm testing this code to accommodate, but no success so far.

  if (currentYearOffsetFromEnd >= 22) {
      activeOffset--;
  }
@mmalerba

I would try to figure out why that off by one thing is happening in the first place rather than just adding an if to decrement it.

activeOffset = 0;
}
if (!this._minDate && this._maxDate || this._minDate && this._maxDate) {

This comment has been minimized.

@mmalerba

mmalerba Apr 21, 2018

Contributor

This can be simplified to if (this._maxDate)

}
if (!this._minDate && this._maxDate || this._minDate && this._maxDate) {
let yearOffset = activeYear - this._dateAdapter!.getYear(this._maxDate);

This comment has been minimized.

@mmalerba

mmalerba Apr 21, 2018

Contributor

Prefer const over let. Also I don't think you need the ! after _dateAdapter

if (!this._minDate && this._maxDate || this._minDate && this._maxDate) {
let yearOffset = activeYear - this._dateAdapter!.getYear(this._maxDate);
let currentYearOffsetFromEnd = (yearOffset - Math.floor(yearOffset)) * yearsPerPage;
activeOffset = (this._minDate) ? 0 : currentYearOffsetFromEnd;

This comment has been minimized.

@mmalerba

mmalerba Apr 21, 2018

Contributor

Don't need () around this._minDate

fix(date-picker): Updates avalible years to be dynamic to min and max
This removes the years which were originally greyed out. Brings in 4
edge cases:
1. No min / max: Standard Behavior
2. Only min: offset to 0 (display min year as first)
3. Only max: Display all paged years minus 1.
4. Both min and max: Same as 3.
@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 22, 2018

Still debugging, but one note that I noticed is that when your _maxDate is lesser than the current Date, then the activeYear is assigned the max's year.
Ex: say _maxDate is 12/12/14, then the activeYear is 2014. This is what could be causing the offset to be sporadic at times.

I'm convinced this is partially the issue, looking more into. Unsure if that is a valid analysis / finding or if it doesn't relate at all.

@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 22, 2018

Here are the current use cases, which from my testing appear to be done (still need to correct / update header):

  1. No Min / Max Dates

screen shot 2018-04-22 at 4 01 24 pm

  1. Only Min Date

screen shot 2018-04-22 at 4 01 43 pm

  1. Only Max Date

screen shot 2018-04-22 at 4 02 33 pm

  1. Both Min and Max Dates

screen shot 2018-04-22 at 4 02 12 pm

@raygervais

This comment has been minimized.

Contributor

raygervais commented May 12, 2018

Question, what is the best approach to handling the date-headings component? I'd like to complete this PR but at the moment I cannot think of a good approach for the last portion (updating the headings).

@mmalerba

This comment has been minimized.

Contributor

mmalerba commented May 24, 2018

@raygervais sorry I haven't had a chance to look into this. I asked my intern to take a look, she created a new PR based off this plus a separate PR where I had fixed some change detection issues, feel free to comment on her PR here: #11485

@raygervais

This comment has been minimized.

Contributor

raygervais commented May 24, 2018

That's quite alright @mmalerba, honestly I was starting to come to terms that I had bitten off too much than I could handle at the time with quite a few out-side-the-computer events going on for me to truly dedicate focus and enthusiasm this direction. Happy to pass this on to your intern and continue contributing where possible in the close future!

I see that the Google Bot is saying that I have to authorize the use of my commits / branch being forked, how would I do so that there is no friction for anyone else working on it?

@mmalerba

This comment has been minimized.

Contributor

mmalerba commented May 24, 2018

@raygervais if you could just comment on the PR saying that you're ok with your commits being used that would be great, thanks!

@JaegerCaiser

This comment has been minimized.

JaegerCaiser commented Jun 5, 2018

I have a question about this, but fisrt just to be clear and understand the situation, what was propose is dont show the year conform the min and max is setted?

Why dont just block the click in the year?

Because a have a situation that my selection date start in the year view, and I select the month at follow, and close datepicker to just select the year and month. But the click works same when the year is disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment