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

Calendar shows wrong month #8173

Closed
wnvko opened this issue Sep 17, 2020 · 11 comments · Fixed by #8509
Closed

Calendar shows wrong month #8173

wnvko opened this issue Sep 17, 2020 · 11 comments · Fixed by #8509
Assignees
Labels
🐛 bug Any issue that describes a bug 📆 calendar 🧨 severity: medium version: 10.1.x ✅ status: resolved Applies to issues that have pending PRs resolving them, or PRs that have already merged.

Comments

@wnvko
Copy link
Contributor

wnvko commented Sep 17, 2020

Description

When value is set in IgxCalendar template, calendar does not show the correct month and year.

  • igniteui-angular version: 10.1
  • browser: all

Steps to reproduce

  1. Open this sample.
  2. Observe shown month and year.

Result

Calendar shows current day month and year.

Expected result

Calendar should scroll to the date representing the value, e.g. in the attached sample it should show January 1958.

@wnvko wnvko added 🐛 bug Any issue that describes a bug version: 10.1.x labels Sep 17, 2020
@MonikaKirkova MonikaKirkova added 🛠️ status: in-development Issues and PRs with active development on them and removed 🆕 status: new labels Oct 22, 2020
@zdrawku zdrawku assigned ddincheva and unassigned zdrawku Nov 2, 2020
@zdrawku zdrawku added ✅ status: resolved Applies to issues that have pending PRs resolving them, or PRs that have already merged. and removed 🛠️ status: in-development Issues and PRs with active development on them labels Nov 2, 2020
@radomirchev radomirchev added this to Validation in 02-Nov-20 M14 Sprint 1 Nov 2, 2020
kdinev added a commit that referenced this issue Nov 4, 2020
fix(calendar): show right month when value is set for 9.1.x #8173
kdinev added a commit that referenced this issue Nov 4, 2020
fix(calendar): show right month when value is set for 10.2.x #8173
kdinev added a commit that referenced this issue Nov 5, 2020
fix(calendar): show right month when value is set for master #8173
@radomirchev radomirchev moved this from Validation to Done in 02-Nov-20 M14 Sprint 1 Nov 13, 2020
@hanastasov hanastasov reopened this Jan 12, 2021
@hanastasov
Copy link
Contributor

hanastasov commented Jan 12, 2021

The calendar has both the value and the viewDate properties. By definiton, viewDate defaults to current day if not explicitly set, and determines what month will be displayed in the calendar.

At the same time, value holds the selected date, if any, and I do not see anywhere in our docs that the selected date should determine the displayed month.

By making this change we are making the viewDate dependent on value, which controverses what our API docs say.

IMO this change should be reverted.

@wnvko let me know what you think.

@ddincheva
Copy link
Contributor

ddincheva commented Jan 12, 2021

The calendar has bith the value and the viewDate properties. By definiton, viewDate defaults to current day if not explicitly set, and determines what month will be displayed in the calendar.

At the same time, value holds the selected date, if any, and I do not see anywhere in our docs that the selected date should determine the displayed month.

By making this change we are making the viewDate dependent on value, which controverses what our API docs say.

IMO this change should be reverted.

In the sample linked in this issue, there is no viewDate set only a value. If the viewDate is set its value is taken. This bug fix is related to the scenario when there is no viewDate set, only a value.

@hanastasov
Copy link
Contributor

In the sample linked in this issue there is no viewDate set only a value. If the viewDate is set its value is taken. This bug fix is related to the scenarion when there is no viewDate set, only a value.

The API docs say that viewDate defaults to current day if not set, but actually it is made to default on the value, if present. This seems incorrect to me.

@wnvko
Copy link
Contributor Author

wnvko commented Jan 12, 2021

I do not know what is correct. IMO if:

  • you have no value nor viewDate it should open calendar in today.
  • you have value but have no viewDate it should open calendar as set in value.
  • you have viewDate it should open calendar as set in viewDate.
    Again this is how I think is correct. To solve this we should ping UI/UX guys.

@hanastasov
Copy link
Contributor

hanastasov commented Jan 12, 2021

@StefanIvanov could you please share the UX view point? Basically we have a dedicated viewDate property that determines what month is displayed in the calendar. Before this change, and according to API docs, viewDate will fallback to the current day, if not set explicitly.

Current behavior, after this change, makes viewDate dependent on value, if value is set. This makes our API docs wrong on the behavior. Also, it makes it hard to apply same logic if we have range, especially multi selection (which value among the N number selected dates would serve as the viewDate).

If we are not sure about it, and to stay compliant with what the API say, I suggest reverting the current change. At least the API is clear on that.

@StefanIvanov
Copy link
Contributor

StefanIvanov commented Jan 13, 2021

From a pure UX/UI point of view, the bug fix makes sense because it enables a scenario where the calendar header is used to show the current selection. Talking about the calendar we may have it embedded in a page or show from any element e.g. a button and that would not mean that the end-user will have a clear indication elsewhere about the selected values. For the case where we have a range selection, I think the header should display a value e.g. Mon, Jan 1 - Mon, Jan 5 which is of course as long as a viewDate is not set. So in short, I am against reverting the change and what we have to do instead is update our API docs accordingly.

@hanastasov
Copy link
Contributor

hanastasov commented Jan 13, 2021

Talking about the calendar we may have it embedded in a page or show from any element e.g. a button and that would not mean that the end-user will have a clear indication elsewhere about the selected values.

I understand that, but for me it seems like we are presuming like selection is the main purpose of the calendar, which dictates the behavior. If this is true, then fine. But then maybe special dates should also dictate the behavior.

Let's extend on this example: a button brings up the calendar, and the user would like to see if there are any selected dates in the current month.

What is more important for the user - to see if there are any selected dates, or if the current month has any selected date. I guess it depends on the business case.

In the end, we have a property controlling this behavior and we are changing its value behind the curtains.

@StefanIvanov
Copy link
Contributor

@hanastasov in your example I think with the introduced behavior the user can see both by checking the header for the selection and by navigating to the current month via the calendar UI and seeing what is marked on the days view.

@hanastasov
Copy link
Contributor

hanastasov commented Jan 14, 2021

@hanastasov in your example I think with the introduced behavior the user can see both by checking the header for the selection and by navigating to the current month via the calendar UI and seeing what is marked on the days view.

And with the previous behavior user was able to see both in one screen (header indicates the selected value), without additional interaction with the UI. Sounds better to me.

Anyway I agree that when single selection, it is good to have the calendar opened in the selected month. I will open a new issue regarding the API docs change.

@hanastasov
Copy link
Contributor

#8173 the issue for correcting the API docs.

@StefanIvanov
Copy link
Contributor

@hanastasov thanks for taking care of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Any issue that describes a bug 📆 calendar 🧨 severity: medium version: 10.1.x ✅ status: resolved Applies to issues that have pending PRs resolving them, or PRs that have already merged.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants